linux-kernel - Re: [PATCH] tty: vt: keyboard: Fix uninitialized variables in vt_do_kdgkb

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: <2025041145-plating-unlined-edf1@gregkh>
Date: Fri, 11 Apr 2025 16:15:45 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Purva Yeshi <purvayeshi550@...il.com>
Cc: jirislaby@...nel.org, tglx@...utronix.de, hdegoede@...hat.com,
	mingo@...nel.org, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] tty: vt: keyboard: Fix uninitialized variables in
 vt_do_kdgkb_ioctl

On Fri, Apr 11, 2025 at 06:48:13PM +0530, Purva Yeshi wrote:
> On 11/04/25 16:58, Greg KH wrote:
> > On Fri, Apr 11, 2025 at 04:45:48PM +0530, Purva Yeshi wrote:
> > > Fix Smatch-detected issue:
> > > 
> > > drivers/tty/vt/keyboard.c:2106 vt_do_kdgkb_ioctl() error:
> > > uninitialized symbol 'kbs'.
> > > drivers/tty/vt/keyboard.c:2108 vt_do_kdgkb_ioctl() error:
> > > uninitialized symbol 'ret'.
> > > 
> > > Fix uninitialized variable warnings reported by Smatch in
> > > vt_do_kdgkb_ioctl(). The variables kbs and ret were used in the kfree
> > > and return statements without guaranteed initialization paths, leading to
> > > potential undefined behavior or false positives during static analysis.
> > > 
> > > Initialize char *kbs to NULL and int ret to -EINVAL at declaration.
> > > This ensures safe use of kfree(kbs) and return ret regardless of control
> > > flow. Also add a default case in the switch to preserve fallback behavior.
> > 
> > When you say "also" in a patch, that is a HUGE flag that this should be
> > split up into a separate change.  Please do that here, don't mix changes
> > that have nothing to do with each other together into one.
> > 
> > Also, why isn't the compilers noticing that these are uninitialized
> > variables?  Are you sure the warning is correct?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> Thank you for the feedback.
> 
> Got it. I will remove the default case from this patch and resend it with
> only the fix for the uninitialized variables.
> 
> Yes, Smatch reports uninitialized variable warnings for kbs and ret because,
> in the function vt_do_kdgkb_ioctl(), both variables are used outside the
> switch block but are only initialized conditionally within certain case
> branches. If the cmd value passed to the function does not match any of the
> explicitly handled cases (KDGKBSENT or KDSKBSENT), then the switch body is
> skipped entirely. In such a scenario, kbs remains uninitialized, yet
> kfree(kbs) is still called, which could result in undefined behavior.

But can that ever really happen?  And if so, how have we never noticed
that before?  And why doesn't gcc/clang warn of this?

> Similarly, ret is returned at the end of the function even though it may not
> have been assigned a value, leading to unpredictable results.

Again, are you sure that can happen?  Please walk through the code paths
to verify this.

thanks,

greg k-h