linux-kernel - Re: [PATCH v2] p54: Fix an error handling path in p54spi

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: <9fa854e1-ad88-9c18-ca68-5709dc1c7906@gmail.com>
Date:   Thu, 16 Jun 2022 15:13:26 +0200
From:   Christian Lamparter <chunkeey@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "John W. Linville" <linville@...driver.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>,
        Johannes Berg <johannes@...solutions.net>
Subject: Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()

On 16/06/2022 12:36, Dan Carpenter wrote:
> On Wed, Jun 15, 2022 at 11:03:34PM +0200, Christian Lamparter wrote:
>> On 13/06/2022 22:57, Christophe JAILLET wrote:
>>> Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
>>>> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
>>>> <christophe.jaillet@...adoo.fr> wrote:
>>>>>
>>>>> If an error occurs after a successful call to p54spi_request_firmware(), it
>>>>> must be undone by a corresponding release_firmware() as already done in
>>>>> the error handling path of p54spi_request_firmware() and in the .remove()
>>>>> function.
>>>>>
>>>>> Add the missing call in the error handling path and remove it from
>>>>> p54spi_request_firmware() now that it is the responsibility of the caller
>>>>> to release the firmawre
>>>>
>>>> that last word hast a typo:  firmware. (maybe Kalle can fix this in post).
>>>
>>> More or less the same typo twice in a row... _Embarrassed_
>>>
>>>>
>>>>> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
>>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>>>> Acked-by: Christian Lamparter <chunkeey@...il.com>
>>>> (Though, v1 was fine too.)
>>>>> ---
>>>>> v2: reduce diffstat and take advantage on the fact that release_firmware()
>>>>> checks for NULL
>>>>
>>>> Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
>>>> could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
>>>> so the p54spi.c could shrink another 5-6 lines.
>>>>
>>>> I think leaving p54spi_request_firmware() callee to deal with
>>>> releasing the firmware
>>>> in the error case as well is nicer because it gets rid of a "but in
>>>> this case" complexity.
>>>
>>>
>>> Take the one you consider being the best one.
>>
>> well said!
>>
>>>
>>> If it deserves a v3 to axe some lines of code, I can do it but, as said
>>> previously,
>>> v1 is for me the cleaner and more future proof.
>>
>> Gee, that last sentence about "future proof" is daring.
> 
> The future is vast and unknowable but one thing which is pretty likely
> is that Christophe's patch will introduce a static checker warning.  We
> really would have expected a to find a release_firmware() in the place
> where it was in the original code.  There is a comment there now so no
> one is going to re-add the release_firmware() but that's been an issue
> in the past.
> 
> I'm sort of surprised that it wasn't a static checker warning already.
> Anyway, I'll add this to Smatch check_unwind.c
> 
> +         { "request_firmware", ALLOC, 0, "*$", &int_zero, &int_zero},
> +         { "release_firmware", RELEASE, 0, "$"},

hmm? I don't follow you there. Why should there be a warning "now"?
(I assume you mean with v2 but not with v1?). If it's because the static
checker can't look beyond the function scope then this would be bad news
since on the "success" path the firmware will stick around until
p54spi_remove().

The p54* drivers would need to be updated (they are ooold) to make good
use of the firmware_cache. There must have been a good reason why it
was designed that way. Especially because the firmware_class uses devm
for the cache already internally. (thanks for Johannes for pointing this
out).

Cheers,
Christian