Re: [PATCH v2] HID: wacom: Prevent potential null dereference after disconnect

[Jason Gerecke]

On Wed, Oct 8, 2014 at 5:28 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> On Wed, Oct 08, 2014 at 05:24:32PM -0700, Jason Gerecke wrote:
>> On Wed, Oct 8, 2014 at 2:40 PM, Dmitry Torokhov
>> <dmitry.torokhov@xxxxxxxxx> wrote:
>> > On Wed, Oct 08, 2014 at 11:25:42AM -0700, Jason Gerecke wrote:
>> >> Repeated connect/disconnect cycles under GNOME can trigger an occasional
>> >> OOPS from within e.g. wacom_led_select_store, presumably due to a timing
>> >> issue where userspace begins setting a value immediately before the
>> >> device disconnects and our shared data is whisked away.
>> >>
>> >> Signed-off-by: Jason Gerecke <killertofu@xxxxxxxxx>
>> >> ---
>> >> Changes in v2:
>> >>  * Added in missing escape character
>> >>
>> >>  drivers/hid/wacom_sys.c | 13 ++++++++++++-
>> >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> >> index 8593047..265429b 100644
>> >> --- a/drivers/hid/wacom_sys.c
>> >> +++ b/drivers/hid/wacom_sys.c
>> >> @@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
>> >>       unsigned int id;
>> >>       int err;
>> >>
>> >> +     if (!wacom)
>> >> +             return -ENODEV;
>> >> +
>> >
>> > Strong NAK. If device could disappear before this check it could as well
>> > disappear after your check.
>> >
>> > This patch does not solve anything.
>> >
>>
>> I assume I'll want to either disable interrupts or take a lock
>> depending on if `wacom_remove` is called from within the interrupt
>> context, but I haven't had to deal with concurrency in the kernel
>> before so I'm not entirely sure which option (or which primitive if
>> locking) would be appropriate...
>
> Actually the sysfs core should not allow anyone descend into sysfs
> show/store methods once you return from sysfs_remove*(). So you need to
> make sure that pointer is valid until then.
>
> Thanks.
>
> --
> Dmitry

Hmm. That's odd. The `wacom_remove` function calls
`wacom_destroy_leds` (which is responsible for removing those sysfs
nodes) prior to calling `wacom_remove_shared_data` (which is
responsible for freeing that pointer). I could imagine that a
disconnect which occurred after the sysfs checks were satisfied but
before our function was called would be able to get around that, but I
don't know if the kernel can be interrupted while the sysfs write is
being handled. I'll double-check what the actual state of things is
when the OOPS happens...

Jason
---
Now instead of four in the eights place /
youâve got three, âCause you added one  /
(That is to say, eight) to the two,     /
But you canât take seven from three,    /
So you look at the sixty-fours....
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/