Re: [PATCH] HID: tighten ioctl command parsing

[Benjamin Tissoires]

On Jul 11 2025, Arnd Bergmann wrote:
> On Fri, Jul 11, 2025, at 11:40, Benjamin Tissoires wrote:
> >
> > On Jul 11 2025, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <arnd@xxxxxxxx>
> >> 
> >> The handling for variable-length ioctl commands in hidraw_ioctl() is
> >> rather complex and the check for the data direction is incomplete.
> >> 
> >> Simplify this code using a switch() statement with the size masked
> >> out, to ensure the rest of the command is correctly matched.
> >
> > How much "urgent" you believe this patch is. I would say 6.17 material,
> > but I'm not sure if your analysis regarding "the check for the data
> > direction is incomplete." would justify a 6.16 late fix.
> 
> I'm not aware of anything being actively broken without my patch,
> the driver just accepts extra commands that it should reject instead.
> 
> My feeling is that we still want the change backported to stable
> kernels as it does address incorrect behavior, and based on that
> it should be in a fixes branch for the current release rather than
> wait for the merge window.
> 
> On the other hand, it needs to be properly tested to ensure
> I'm not breaking things.
> 
> > Also, lately I added a new tools/testing/selftests/hid/hidraw.c to test
> > the latest HIDIOCREVOKE addition. I wonder if we should not add a couple
> > of checks there to ensure we get the different kind of other ioctls
> > tested before and after this patch.

FWIW, I was told to use more AI in my company, and I was too lazy to
write a full coverage. So I asked claude to help me out on that and the
result is pasted down below. Feel free to include them in v2 (note:
checkpatch complains about the co-developed-by tag, but not sure we have
a concensus for that ATM).

> 
> Yes, makes sense, e.g. testing HIDIOCGFEATURE with incorrect _IOC_DIR
> bits would be a useful addition.

The current behaviour for HIDIOCGRAWNAME with a wrong _IOC_DIR bit is to
return -EINVAL, while with your patch applied it's changed to -ENOTTY.
Is that expected?

---