linux-kernel - Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers

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]

Date:   Sat, 11 Feb 2023 23:27:45 +0100
From:   Pietro Borrello <borrello@...g.uniroma1.it>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Jiri Kosina <jikos@...nel.org>, Hanno Zulla <kontakt@...no.de>,
        Carlo Caione <carlo@...lessm.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>, Jakob Koschel <jkl820.git@...il.com>,
        Hillf Danton <hdanton@...a.com>,
        Roderick Colenbrander <roderick@...kai.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers

On Fri, 10 Feb 2023 at 15:26, Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
> [...]
> >
> > -     if (bigben->removed || !report_field)
>
> You are removing an important test here: if (!report_field), please keep
> it.

To my understanding, that check was added in commit
918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
to prevent the NULL pointer crash, only fixing the crash point.
However, the true root cause was a missing check for output
reports patched in commit
c7bf714f8755 ("HID: check empty report_list in bigben_probe()"),
where the type-confused report list_entry was overlapping with
a NULL pointer, which was then causing the crash.
Let me know if there is any other path that may result in having a
report with no fields. In that case, it would make sense to keep the
check.

>
> > -             return;
> > +     spin_lock_irqsave(&bigben->lock, flags);
> >
> >       if (bigben->work_led) {
> >               bigben->work_led = false;
> > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> >               report_field->value[7] = 0x00; /* padding */
> >               hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> >       }
> > +
> > +     spin_unlock_irqrestore(&bigben->lock, flags);
>
> Ouch, having hid_hw_request() called whithin a spinlock is definitely not
> something that should be done.
>
> However, the spinlock should be protecting 2 kinds of things:
> - any access to any value of struct bigben_device, but in an atomic way
>   (i.e. copy everything you need locally in a spinlock, then release it
>   and never read that struct again in that function).
> - the access to bigben->removed, which should be checked only in
>   bigben_schedule_work() and in the .remove() function.
>
> Please note that this is what the playstation driver does: it prepares
> the report under the spinlock (which is really fast) before sending the
> report to the device which can be slow and be interrupted.
>
> With that being said, it is clear that we need 2 patches for this one:
> - the first one introduces the spinlock and protects the concurrent
>   accesses to struct bigben_device (which is roughly everything below
>   with the changes I just said)
> - the second one introduces bigben_schedule_work() and piggy backs on
>   top of that new lock.

Thanks for clarifying. I will work on a v4 patch.
Best regards,
Pietro