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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VdY8aoF+dCm0j7EA-53BFaWdepvYYv7h5MVz2kVWagOOA@mail.gmail.com>
Date:   Sat, 6 Nov 2021 20:35:58 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Ajay Garg <ajaygargnsit@...il.com>
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.

On Sat, Nov 6, 2021 at 6:50 PM Ajay Garg <ajaygargnsit@...il.com> wrote:
>
>
> v1 patch at :
> https://lore.kernel.org/linux-serial/YYZN30qfaKMskVwE@kroah.com/T/#t
>
>
> Changes in v2 :
>
>         * Changes as required by scripts/checkpatch.pl
>
>         * Checking whether kbs is not NULL before kfree is not required,
>           as kfree(NULL) is safe. So, dropped the check.
>
>
> For brevity, here is the background :
>
>
> In "vt_do_kdgkb_ioctl", kbs is kmalloced, if cmd is one of KDGKBSENT or
> KDSKBSENT.
>
> If cmd is none of the above, kbs is not kmalloced, and runs
> direct to kfree(kbs).
>
> Values of local-variables on the stack can take indeterminate values,
> so we initialize kbs to NULL. Then, if kbs is not kmalloced, we have
> kfree(NULL) at the last.

> Note that kfree(NULL) is safe.

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. 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.

-- 
With Best Regards,
Andy Shevchenko