linux-kernel - Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver

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: <CAH2-hcLOA0ECg7ubKh_r9BryRuGPjJ1Cv1ErUpMmQ+Yag5qnWA@mail.gmail.com>
Date:   Thu, 27 Apr 2023 10:13:22 +0200
From:   Tomáš Mudruňka <tomas.mudrunka@...il.com>
To:     Jeff LaBundy <jeff@...undy.com>
Cc:     dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver

Hello, thanks for your notes.

> This is the right idea. I am sort of splitting hairs here, however I
> think it makes sense to place this read before the IRQ is requested
> and not after.
>
>
> As written, there is room for an ever-so-tiny race condition wherein
> the IRQ is asserted just after it is requested. Before the threaded
> handler has run however, the new read in probe swallows the IRQ status
> before the threaded handler can read it and react to errors.

In fact i believe quite the opposite case to be true.
If i read before registering IRQ there will be ever-so-tiny race condition that
would allow to miss the edge (exactly the bug this patch is fixing,
but limited).

In the case you describe the worst scenario is likely that the interrupt handler
will be called only to re-read status and immediately return on this condition:

if (!status) return IRQ_NONE;

> Also, I think you should at least capture and evaluate lm8333_read8()'s
> return value as is already done for the calls to lm8333_write8().

Well. If you think this will bring any benefits, i might as well just call
lm8333_irq_thread() instead of lm8333_read8()
Would that be acceptable solution?

Tom.