linux-kernel - Re: [PATCH] HID: wacom: Add parse before start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAO-hwJJZdSnJ1ysOk_UACsxwhg-1u3SEg8rvykCEaHzqY5-pmQ@mail.gmail.com>
Date:   Fri, 5 Nov 2021 19:36:40 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>
Cc:     Jiri Kosina <jikos@...nel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: wacom: Add parse before start

Hi Jiasheng,

On Thu, Nov 4, 2021 at 8:52 AM Jiasheng Jiang <jiasheng@...as.ac.cn> wrote:
>
> It might be better to add  hid_parse() before
> wacom_parse_and_register() to ask for the report descriptor
> like what wacom_probe() does.
>
> Fixes: 471d171 ("Input: wacom - move the USB (now hid) Wacom driver in drivers/hid")
> Signed-off-by: Jiasheng Jiang <jiasheng@...as.ac.cn>
> ---
>  drivers/hid/wacom_sys.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 57bfa0a..48cb2e4 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2486,6 +2486,9 @@ static void wacom_wireless_work(struct work_struct *work)
>
>                 wacom_wac1->pid = wacom_wac->pid;
>                 hid_hw_stop(hdev1);
> +               error = hid_parse(wacom1->hdev);
> +               if (error)
> +                       goto fail;

I am pretty sure we don't need those calls (everywhere in this patch).

hid_parse() has the effect of requesting the transport layer to pull
the report descriptor and then parses it at the hid core level.
However, we are called here in callbacks after wacom_probe() has been
called already once for those devices (wacom1 and wacom2).
So basically, hid_parse() is called in wacom_probe(), we store the hid
device in a shared space in the driver, and then when the work is
called because a new device is connected, we just pull that hid device
(already parsed) and present it to the userspace.

Another fact that makes me think we are already doing the right thing
is that if hid_parse() is not called on a hid device, we have a
segfault while processing the events. And here, we don't.

Cheers,
Benjamin

>                 error = wacom_parse_and_register(wacom1, true);
>                 if (error)
>                         goto fail;
> @@ -2498,6 +2501,9 @@ static void wacom_wireless_work(struct work_struct *work)
>                                 *((struct wacom_features *)id->driver_data);
>                         wacom_wac2->pid = wacom_wac->pid;
>                         hid_hw_stop(hdev2);
> +                       error = hid_parse(wacom2->hdev);
> +                       if (error)
> +                               goto fail;
>                         error = wacom_parse_and_register(wacom2, true);
>                         if (error)
>                                 goto fail;
> @@ -2710,12 +2716,18 @@ static void wacom_mode_change_work(struct work_struct *work)
>         }
>
>         if (wacom1) {
> +               error = hid_parse(wacom1->hdev);
> +               if (error)
> +                       return;
>                 error = wacom_parse_and_register(wacom1, false);
>                 if (error)
>                         return;
>         }
>
>         if (wacom2) {
> +               error = hid_parse(wacom2->hdev);
> +               if (error)
> +                       return;
>                 error = wacom_parse_and_register(wacom2, false);
>                 if (error)
>                         return;
> --
> 2.7.4
>