netdev - Re: ath10k: ret used but uninitialized

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]

Date:   Fri, 7 Jul 2017 16:15:58 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Kalle Valo <kvalo@....qualcomm.com>,
        Erik Stromdahl <erik.stromdahl@...il.com>,
        "ath10k@...ts.infradead.org" <ath10k@...ts.infradead.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: ath10k: ret used but uninitialized

Hi Arnd,

On Fri, Jul 7, 2017 at 4:14 PM, Arnd Bergmann <arnd@...db.de> wrote:
> On Fri, Jul 7, 2017 at 12:04 PM, Kalle Valo <kvalo@....qualcomm.com> wrote:
>> Erik Stromdahl <erik.stromdahl@...il.com> writes:
>>>> With gcc 4.1.2:
>>>>
>>>> drivers/net/wireless/ath/ath10k/sdio.c: In function
>>>> ‘ath10k_sdio_mbox_rxmsg_pending_handler’:
>>>> drivers/net/wireless/ath/ath10k/sdio.c:676: warning: ‘ret’ may be used
>>>> uninitialized in this function
>>>>
>>>>> +
>>>>> +       *done = true;
>>>>> +
>>>>> +       /* Copy the lookahead obtained from the HTC register table into our
>>>>> +        * temp array as a start value.
>>>>> +        */
>>>>> +       lookaheads[0] = msg_lookahead;
>>>>> +
>>>>> +       timeout = jiffies + SDIO_MBOX_PROCESSING_TIMEOUT_HZ;
>>>>
>>>> Although very unlikely due to the long timeout, if the code is preempted here,
>>>> and the loop below never entered, ret will indeed be uninitialized.
>>>>
>>>> It's unclear to me what the proper initialization would be, though, so
>>>> that's why I didn't send a patch.
>>>>
>>> I think it would be best to use 0 as initial value of ret in this case.
>>> This will make all other interrupts be processed in a normal way.
>>>
>>> Kalle: Should I create a new patch (initializing ret with zero)?
>>
>> Yes, please send a new patch fixing this.
>>
>> But I don't like that much with the style of initialising ret to zero,
>> it tends to hide things. Instead my preference is something like below
>> where the error handling is more explicit and easier to find where it's
>> exactly failing. But that's just an example how I would try to solve it,
>> it still lacks the handling of -ECANCEL etc.
>
> I think I would simply replace the "while() {}" loop with "do{} while()",
> as that would guarantee it to be run at least once in a way that the
> compiler can see.

Right, that's probably the simplest and cleanest solution.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds