[Spice-devel] [usbredir PATCH] usbredirfilter_check: block device if all its interfaces skipped

[Hans de Goede]

Hi,

On 08-07-15 18:22, Uri Lublin wrote:
> Hi Hans,
>
> Thanks for the quick review.
> See answers below
>
> On Wed, 2015-07-08 at 15:45 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 08-07-15 15:26, Uri Lublin wrote:
>>> See usbredirfilter.h for when interfaces are skipped.
>>>
>>> Fixes rhbz#1179210
>>
>> Sigh, the kvm in question is really messed up as it uses non
>> bootclass hid interfaces,
>> that means it won't work in many BIOS' etc. What a mess ...
>>
>>> ---
>>>    usbredirparser/usbredirfilter.c | 11 ++++++++---
>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/usbredirparser/usbredirfilter.c
>>> b/usbredirparser/usbredirfilter.c
>>> index ef9c63a..5cbbfbf 100644
>>> --- a/usbredirparser/usbredirfilter.c
>>> +++ b/usbredirparser/usbredirfilter.c
>>> @@ -172,7 +172,7 @@ int usbredirfilter_check(
>>>        uint16_t vendor_id, uint16_t product_id, uint16_t
>>> device_version_bcd,
>>>        int flags)
>>>    {
>>> -    int i, rc;
>>> +    int i, rc, num_skipped=0;
>>>
>>>        if (usbredirfilter_verify(rules, rules_count))
>>>            return -EINVAL;
>>> @@ -190,9 +190,10 @@ int usbredirfilter_check(
>>>        for (i = 0; i < interface_count; i++) {
>>>            if (!(flags & usbredirfilter_fl_dont_skip_non_boot_hid)
>>> &&
>>>                    interface_count > 1 && interface_class[i] == 0x03
>>> &&
>>> -                interface_subclass[i] == 0x00 &&
>>> interface_protocol[i] == 0x00)
>>> +                interface_subclass[i] == 0x00 &&
>>> interface_protocol[i] == 0x00) {
>>> +            num_skipped++;
>>>                continue;
>>> -
>>> +	}
>>>            rc = usbredirfilter_check1(rules, rules_count,
>>> interface_class[i],
>>>                                       vendor_id, product_id,
>>> device_version_bcd,
>>>                                       flags &
>>> usbredirfilter_fl_default_allow);
>>> @@ -200,6 +201,10 @@ int usbredirfilter_check(
>>>                return rc;
>>>        }
>>>
>>> +    /* If all interfaces were skipped, block the device */
>>> +    if (num_skipped == interface_count)
>>> +	return -ENOENT;
>>> +
>>>        return 0;
>>>    }
>>
>> This seems wrong, this means that if a user wants to redirect some
>> custom hid device,
>> with just a single hid interface that it will always be blocked
>> because of this.
>
> In that case interface_count==1 and it will not be skipped.

Right, so make it a device with 2 custom hid interfaces ...

> If that's not enough, see more below.
>
>>
>> I suggest instead adding a vid/pid based list of devices on which to
>> not skip non
>> boot compliant hid devices. This way if hid devices are allowed to be
>> redirected
>> by the filter, the device can still be redirected, and in the default
>> case where
>> hid devices are not allowed, we will skip the non-boot-hid skipping,
>> do the regular
>> hid check, and block the device based on that.
>
> That may work but I think it is complicated for users.
> If they are going to use specific vid/pid, they might as
> well add a filter-rule for those specific devices.

The skip in the filter is there so that for example a usb headset which
has both an audio interface and a usb=hid interface for the volume buttons
will not get filtered out as being a hid device.

> Also assume the filter used is block-all-devices
> (-1,-1,-1,-1, 0).

No by default for auto-redirect which is the problem here we use a filter
which filters out hid devices. Normally this works fine to not auto redirect
keyboards and mice since they have a hid interface which had an usb keyboard
boot interface subclass and as such will not trigger the skip checking for
this interface code. The problem here seems to be that a kvm is used which
only has non bootclass hid interfaces.

> This will not be enough and the user
> would have to provide the specific vid/uid list of such
> weird devices (after failing the first time).

No the idea is to have a blacklist inside usbredir for devices for which
the skip code should be skipped, the user will not have to do anything.

Can you ask the reporter to provide lsusb -v output for the usb interface
of the kvm in question, then we can better analyse what exactly is going
wrong here. Perhaps my analyses of the problem is wrong.

Regards,

Hans

p.s.

I'm on vacation from July 11th - July 19th, so if I'm quiet that is why :)