linux-kernel - Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

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]

Message-ID: <CAHP4M8Vdj4Eb8q773BeHvsW9n6t=3n1WznuXAR4fZCNi1J6rOg@mail.gmail.com>
Date:   Sun, 7 Nov 2021 00:29:01 +0530
From:   Ajay Garg <ajaygargnsit@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Pavel Skripkin <paskripkin@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>, jirislaby@...nel.org,
        kernel@...il.dk
Cc:     "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs)
 runs fine even if kbs is not kmalloced.

First of all, many thanks to Pavel for all the help and guidance,
nature bless you.
I will make it a point to keep all maintainers in loop, for all my
future patches.

>
> Everybody who is developing for kernel may check this easily, no need
> to have this in the commit message.
>
> As I told you, NAK.
> This is no value in this patch according to the commit message.
>
> If you have a compiler warning you need to provide the command line
> for `make` that makes that warning appear.

"make" as such runs fine, without any blocker (on my x86_64 machine).

The "kbs not initialized" is seen, when the smatch static-analyzer is run.
Thereafter, the patch was floated, to make the method
"vt_do_kdgkb_ioctl" crash-proof by handling kbs properly, without
depending upon external factors on whether a switch-case is hit or
not.

> In that case the better
> solution would be to add default case because some compilers can make
> (wrong) assumptions based on the absence of the default case.
>
> Something like
>
> default:
>    kbs = NULL;
>    break;
>
> at the end of the switch.
>
> But again, your current commit message does not sell.

Hmm, am not sure what to make of this.

I guess, we could follow one of the following approaches :

i)
Leave things as it is, as there is no blocker in the make-compilation/runtime.

ii)
Put the "default: kbs = NULL; break;" case, as suggested, to ensure
"vt_do_kdgkb_ioctl" does its work fine flawlessly, without depending
upon external clients.

Since there is no blocker in functionality, I am ok with whatever
consensus is reached by the maintainers.
If we wish to go with ii), please let me know, I will float next
patch-version with the ii) change, along with a better commit-message.

As of now, since Andy has voted for a NAK, I would not pursue this
further as of now :)

Thanks and Regards,
Ajay