v1 patch at :
https://lore.kernel.org/linux-serial/[email protected]/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.
Signed-off-by: Ajay Garg <[email protected]>
---
drivers/tty/vt/keyboard.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index dfef7de8a057..54155fc91cd2 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2049,7 +2049,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
{
unsigned char kb_func;
unsigned long flags;
- char *kbs;
+ char *kbs = NULL;
int ret;
if (get_user(kb_func, &user_kdgkb->kb_func))
--
2.30.2
Hi, Ajay!
On 11/6/21 13:40, Ajay Garg wrote:
>
> v1 patch at :
> https://lore.kernel.org/linux-serial/[email protected]/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 :
>
>
Please, don't put change log into commit message. It should go under ---
> 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.
>
>
These is only one caller of vt_do_kdgkb_ioctl, which simple does:
case KDGKBSENT:
case KDSKBSENT:
return vt_do_kdgkb_ioctl(cmd, up, perm);
It means, that cmd can not be different from KDGKBSENT and KDSKBSENT.
I guess, you found this "issue" via static analysis tool like smatch or
smth similar, but this bug is impossible right now.
>
> Signed-off-by: Ajay Garg <[email protected]>
> ---
<--- here
> drivers/tty/vt/keyboard.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index dfef7de8a057..54155fc91cd2 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2049,7 +2049,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
> {
> unsigned char kb_func;
> unsigned long flags;
> - char *kbs;
> + char *kbs = NULL;
> int ret;
>
> if (get_user(kb_func, &user_kdgkb->kb_func))
>
With regards,
Pavel Skripkin
Hi Pavel,
Thanks for the review.
>
> Please, don't put change log into commit message. It should go under ---
>
Ok, many thanks Pavel.
Will take care in all my future patches.
>
> These is only one caller of vt_do_kdgkb_ioctl, which simple does:
>
>
> case KDGKBSENT:
> case KDSKBSENT:
> return vt_do_kdgkb_ioctl(cmd, up, perm);
>
> It means, that cmd can not be different from KDGKBSENT and KDSKBSENT.
>
> I guess, you found this "issue" via static analysis tool like smatch or
> smth similar, but this bug is impossible right now.
>
Yes, following was reported by smatch (amongst others) :
vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
Regarding the current state, "vt_do_kdgkb_ioctl" should ideally behave
correctly independently, without bothering whether a cmd is a valid
one. From that perspective, it makes sense to ensure that kfree never
crashes.
However, I don't have any strong opinions on what is right or what is
wrong, as long as things work fine.
So, if there is a general consensus that the change should not be made
currently, I would be ok.
In case the change should be made, kindly let me know, I will post the
v3 patch (making change as per the review-comment of moving changelog
below ---).
Thanks and Regards,
Ajay
On 11/6/21 15:16, Ajay Garg wrote:
> Hi Pavel,
>
> Thanks for the review.
>
>>
>> Please, don't put change log into commit message. It should go under ---
>>
>
> Ok, many thanks Pavel.
> Will take care in all my future patches.
>
>
>>
>> These is only one caller of vt_do_kdgkb_ioctl, which simple does:
>>
>>
>> case KDGKBSENT:
>> case KDSKBSENT:
>> return vt_do_kdgkb_ioctl(cmd, up, perm);
>>
>> It means, that cmd can not be different from KDGKBSENT and KDSKBSENT.
>>
>> I guess, you found this "issue" via static analysis tool like smatch or
>> smth similar, but this bug is impossible right now.
>>
>
> Yes, following was reported by smatch (amongst others) :
> vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
>
>
> Regarding the current state, "vt_do_kdgkb_ioctl" should ideally behave
> correctly independently, without bothering whether a cmd is a valid
> one. From that perspective, it makes sense to ensure that kfree never
> crashes.
>
> However, I don't have any strong opinions on what is right or what is
> wrong, as long as things work fine.
>
> So, if there is a general consensus that the change should not be made
> currently, I would be ok.
> In case the change should be made, kindly let me know, I will post the
> v3 patch (making change as per the review-comment of moving changelog
> below ---).
>
>
I can't say if it needed or not, since I am not the maintainer. I've
just said my thoughts on this change. It looks like you missed all
maintainers emails in your CC list :)
└──$ ./scripts/get_maintainer.pl -f drivers/tty/vt/keyboard.c
Greg Kroah-Hartman <[email protected]> (supporter:TTY
LAYER,commit_signer:10/10=100%,authored:2/10=20%,added_lines:31/66=47%,removed_lines:31/59=53%)
Jiri Slaby <[email protected]> (supporter:TTY
LAYER,commit_signer:6/10=60%,authored:3/10=30%,added_lines:14/66=21%,removed_lines:4/59=7%)
Andy Shevchenko <[email protected]>
(commit_signer:4/10=40%,authored:4/10=40%,added_lines:18/66=27%,removed_lines:21/59=36%)
Emil Renner Berthing <[email protected]>
(commit_signer:1/10=10%,authored:1/10=10%,removed_lines:3/59=5%)
[email protected] (open list)
This is the list of people you need to send patches to. Please, use this
script next time to not miss people related to your change :)
The same for your other patch.
With regards,
Pavel Skripkin
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
On Sat, Nov 6, 2021 at 6:50 PM Ajay Garg <[email protected]> wrote:
>
>
> v1 patch at :
> https://lore.kernel.org/linux-serial/[email protected]/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
Thanks Andy, v3-patch has been posted at :
https://lore.kernel.org/linux-serial/[email protected]/T/#u
Many thanks to Pavel and Andy for the review/help throughout.
Let's continue on the v3-patch thread-link.
>
> It’s fine to add default case as I suggested. The problem with your contribution is in the commit message. Selling point would be (according to the below) the smatch complain with the idea of having default case in general.
>
> So something like:
> ===
> The smatch tool gives a warning on the code in ...
>
> warning: ...
>
> 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.
>
> ...
>
> ===
>