Re: [PATCH] tty: vt: keyboard: Fix uninitialized variables in vt_do_kdgkb_ioctl

[Greg KH <gregkh@linuxfoundation.org>]

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