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

[Uri Lublin]

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.

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.

Also assume the filter used is block-all-devices
(-1,-1,-1,-1, 0). 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).

An alternative is to call usbredirfilter_check1 also
for cases that are skipped now, and pass an additional parameter
that means do-not-check-class.
That way a rule that allow/block a specific vid/pid will apply.

Thanks,
    Uri