linux-kernel - Re: [PATCH] hid: logitech: check the return value of create_singlethread

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO-hwJLwvZvhPPKU7r74xtKsp_P5VBZ2964iQhgBR5zF-ZM7wg@mail.gmail.com>
Date:   Mon, 11 Mar 2019 16:08:51 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Kangjie Lu <kjlu@....edu>
Cc:     pakki001@....edu, 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: logitech: check the return value of create_singlethread_workqueue

On Sat, Mar 9, 2019 at 5:43 AM Kangjie Lu <kjlu@....edu> wrote:
>
> create_singlethread_workqueue may fail and return NULL. The fix
> checks if it is NULL to avoid NULL pointer dereference.
>
> Signed-off-by: Kangjie Lu <kjlu@....edu>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 15ed6177a7a3..efbc39b92aa2 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2156,6 +2156,9 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>
>         /* init the hardware command queue */
>         data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
> +       if (!data->wq)
> +               return -ENOMEM;
> +

I agree there is something wrong with the current code, but I think
you are not fixing the issue properly.
The code just above allocates data with kzalloc and stores it in the
hid_device and set up everything. If create_singlethread_workqueue()
fails now, then we are screwed and you need to reverse all of the FF
init.

I think we should move the thread creation above so we are not leaking
memory and not create the FF elements if there is something wrong.

Cheers,
Benjamin

>         atomic_set(&data->workqueue_size, 0);
>
>         /* initialize with zero autocenter to get wheel in usable state */
> --
> 2.17.1
>