[PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Ajay Garg]

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.



Signed-off-by: Ajay Garg <ajaygargnsit@gmail.com>
---
 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

Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Pavel Skripkin]

Hi, Ajay!

On 11/6/21 13:40, Ajay Garg 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 :
> 
> 

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 <ajaygargnsit@gmail.com>
> ---
<--- 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

Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Ajay Garg]

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

Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Pavel Skripkin]

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 <gregkh@linuxfoundation.org> (supporter:TTY 
LAYER,commit_signer:10/10=100%,authored:2/10=20%,added_lines:31/66=47%,removed_lines:31/59=53%)
Jiri Slaby <jirislaby@kernel.org> (supporter:TTY 
LAYER,commit_signer:6/10=60%,authored:3/10=30%,added_lines:14/66=21%,removed_lines:4/59=7%)
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
(commit_signer:4/10=40%,authored:4/10=40%,added_lines:18/66=27%,removed_lines:21/59=36%)
Emil Renner Berthing <kernel@esmil.dk> 
(commit_signer:1/10=10%,authored:1/10=10%,removed_lines:3/59=5%)
linux-kernel@vger.kernel.org (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

Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Andy Shevchenko]

On Sat, Nov 6, 2021 at 6:50 PM Ajay Garg <ajaygargnsit@gmail.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

Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Ajay Garg]

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

Re: [PATCH v2] tty: vt: keyboard: initialize "kbs" so that kfree(kbs) runs fine even if kbs is not kmalloced.

[Ajay Garg]

Thanks Andy, v3-patch has been posted at :
https://lore.kernel.org/linux-serial/20211106220315.392842-1-ajaygargnsit@gmail.com/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.
>
> ...
>
> ===
>