Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
From: Johan Hovold <johan@kernel.org>
Date: 2021-11-08 09:41:52
Also in:
lkml
First, please fix your patch Subject which is way too verbose. You should aim at less than 72 chars including prefix. Something like "vt: keyboard: suppress smatch warning in vt_do_kdgkb_ioctl" should do. On Sun, Nov 07, 2021 at 08:47:21AM +0530, Ajay Garg wrote:
smatch-kchecker gives the following warnings when run on keyboard.c :
vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
vt_do_kdgkb_ioctl() error: uninitialized symbol 'ret'.
This usually happens when switch has no default case and static
analyzers and even sometimes compilers can’t prove that all possible
values are covered.
Thus, the default switch-case has been added, which sets the values
for the two variables :
* kbs as NULL, which also nicely fits in with kfree.
* ret as -ENOIOCTLCMD (on same lines if there is no cmd
match in "vt_do_kdskled" method).Not sure how far we want to take the suppression of false-positive warnings but at least this isn't the right way to do it.
quoted hunk ↗ jump to hunk
Many thanks to the following for review of previous versions : * Pavel Skripkin [off-list ref] * Andy Shevchenko [off-list ref] Signed-off-by: Ajay Garg <redacted> --- There were discussions previously, and the current patch is the result. v1 : https://lore.kernel.org/linux-serial/YYZN30qfaKMskVwE@kroah.com/T/#t (local) v2 : https://lore.kernel.org/linux-serial/CAHP4M8Vdj4Eb8q773BeHvsW9n6t=3n1WznuXAR4fZCNi1J6rOg@mail.gmail.com/T/#m18f45676feaba6b1f01ddd5fe607997b190ef4b9 (local) v3 : https://lore.kernel.org/linux-serial/20211106220315.392842-1-ajaygargnsit@gmail.com/T/#u (local) 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. Changes in v3 : * Using default-switch case, and setting the variables when there is no matching cmd. Changes in v4 : * Removed braces for the default switch-case. drivers/tty/vt/keyboard.c | 6 ++++++ 1 file changed, 6 insertions(+)diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c index c7fbbcdcc346..f66c32fe7ef1 100644 --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c@@ -2090,6 +2090,10 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) ret = 0; break; + default: + kbs = NULL; + ret = -ENOIOCTLCMD; + break; } kfree(kbs);
Instead, move the kfree() into the two cases blocks and initialise ret
to 0 as is done in several other vt helpers in case a driver bug ever
causes them to be called for the wrong cmds (e.g. instead of sprinkling
WARN_ON(1) in all those functions).
You may want to mention that the kfree warning was introduced by
07edff926520 ("vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl")
which moved the shared allocation into the switch statement, and perhaps
also mention
4e1404a5cd04 ("vt: keyboard, extract and simplify vt_kdskbsent")
for the ret warning.
Johan