[PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[Hangyu Hua]

As array_index_nospec's comments indicate，a bounds checking need to add
before calling array_index_nospec.

Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
 drivers/tty/vt/keyboard.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index be8313cdbac3..b9845455df79 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2067,6 +2067,9 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 	if (get_user(kb_func, &user_kdgkb->kb_func))
 		return -EFAULT;
 
+	if (kb_func >= MAX_NR_FUNC)
+		return -EFAULT;
+
 	kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);
 
 	switch (cmd) {
-- 
2.34.1

Re: [PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[Jiri Slaby]

On 08. 09. 22, 9:54, Hangyu Hua wrote:
> As array_index_nospec's comments indicate，a bounds checking need to add
> before calling array_index_nospec.
> 
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>   drivers/tty/vt/keyboard.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index be8313cdbac3..b9845455df79 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2067,6 +2067,9 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>   	if (get_user(kb_func, &user_kdgkb->kb_func))
>   		return -EFAULT;
>   
> +	if (kb_func >= MAX_NR_FUNC)

kb_func is unsigned char and MAX_NR_FUNC is 256. So this should be 
eliminated by the compiler anyway.

But the check might be a good idea if we ever decide to support more 
keys. But will/can we? I am not so sure, so adding it right now is kind 
of superfluous. In any way we'd need to introduce a completely different 
iterface/ioctls.

> +		return -EFAULT;

EINVAL would be more appropriate, IMO.

> +
>   	kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);
>   
>   	switch (cmd) {

thanks,
-- 
js
suse labs

Re: [PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[Greg KH]

On Thu, Sep 08, 2022 at 03:54:03PM +0800, Hangyu Hua wrote:
> As array_index_nospec's comments indicate，a bounds checking need to add
> before calling array_index_nospec.
> 
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>  drivers/tty/vt/keyboard.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index be8313cdbac3..b9845455df79 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2067,6 +2067,9 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  	if (get_user(kb_func, &user_kdgkb->kb_func))
>  		return -EFAULT;
>  
> +	if (kb_func >= MAX_NR_FUNC)
> +		return -EFAULT;

Wrong error to return, only ever return that error if you have a memory
fault from copy_to/from_user().

But even then, how can kb_func ever be greater than MAX_NR_FUNC?  And
what is wrong with it being MAX_NR_FUNC?

> +
>  	kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);

Maybe we really don't need this at all, right?

thanks,

greg k-h

Re: [PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[kernel test robot]

Hi Hangyu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on linus/master v6.0-rc4 next-20220908]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hangyu-Hua/tty-vt-add-a-bounds-checking-in-vt_do_kdgkb_ioctl/20220908-155511
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220908/202209082101.rCth0nPs-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/9878c90ddacf7a81079f77b10502496c4d6cd0cb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hangyu-Hua/tty-vt-add-a-bounds-checking-in-vt_do_kdgkb_ioctl/20220908-155511
        git checkout 9878c90ddacf7a81079f77b10502496c4d6cd0cb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/tty/vt/ fs/ext4/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/tty/vt/keyboard.c:2070:14: warning: result of comparison of constant 256 with expression of type 'unsigned char' is always false [-Wtautological-constant-out-of-range-compare]
           if (kb_func >= MAX_NR_FUNC)
               ~~~~~~~ ^  ~~~~~~~~~~~
   1 warning generated.


vim +2070 drivers/tty/vt/keyboard.c

  2059	
  2060	int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
  2061	{
  2062		unsigned char kb_func;
  2063		unsigned long flags;
  2064		char *kbs;
  2065		int ret;
  2066	
  2067		if (get_user(kb_func, &user_kdgkb->kb_func))
  2068			return -EFAULT;
  2069	
> 2070		if (kb_func >= MAX_NR_FUNC)
  2071			return -EFAULT;
  2072	
  2073		kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);
  2074	
  2075		switch (cmd) {
  2076		case KDGKBSENT: {
  2077			/* size should have been a struct member */
  2078			ssize_t len = sizeof(user_kdgkb->kb_string);
  2079	
  2080			kbs = kmalloc(len, GFP_KERNEL);
  2081			if (!kbs)
  2082				return -ENOMEM;
  2083	
  2084			spin_lock_irqsave(&func_buf_lock, flags);
  2085			len = strlcpy(kbs, func_table[kb_func] ? : "", len);
  2086			spin_unlock_irqrestore(&func_buf_lock, flags);
  2087	
  2088			ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
  2089				-EFAULT : 0;
  2090	
  2091			break;
  2092		}
  2093		case KDSKBSENT:
  2094			if (!perm || !capable(CAP_SYS_TTY_CONFIG))
  2095				return -EPERM;
  2096	
  2097			kbs = strndup_user(user_kdgkb->kb_string,
  2098					sizeof(user_kdgkb->kb_string));
  2099			if (IS_ERR(kbs))
  2100				return PTR_ERR(kbs);
  2101	
  2102			spin_lock_irqsave(&func_buf_lock, flags);
  2103			kbs = vt_kdskbsent(kbs, kb_func);
  2104			spin_unlock_irqrestore(&func_buf_lock, flags);
  2105	
  2106			ret = 0;
  2107			break;
  2108		}
  2109	
  2110		kfree(kbs);
  2111	
  2112		return ret;
  2113	}
  2114	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[Hangyu Hua]

On 8/9/2022 16:10, Jiri Slaby wrote:
> On 08. 09. 22, 9:54, Hangyu Hua wrote:
>> As array_index_nospec's comments indicate，a bounds checking need to add
>> before calling array_index_nospec.
>>
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>> ---
>>   drivers/tty/vt/keyboard.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
>> index be8313cdbac3..b9845455df79 100644
>> --- a/drivers/tty/vt/keyboard.c
>> +++ b/drivers/tty/vt/keyboard.c
>> @@ -2067,6 +2067,9 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry 
>> __user *user_kdgkb, int perm)
>>       if (get_user(kb_func, &user_kdgkb->kb_func))
>>           return -EFAULT;
>> +    if (kb_func >= MAX_NR_FUNC)
> 
> kb_func is unsigned char and MAX_NR_FUNC is 256. So this should be 
> eliminated by the compiler anyway.
> 
> But the check might be a good idea if we ever decide to support more 
> keys. But will/can we? I am not so sure, so adding it right now is kind 
> of superfluous. In any way we'd need to introduce a completely different 
> iterface/ioctls.

If you say so, I think greg should be right. We don't need any bounds 
checking here. It may be a good idea to delete redundant
array_index_nospec. Do i need to make a new patch?

> 
>> +        return -EFAULT;
> 
> EINVAL would be more appropriate, IMO.
> 
>> +
>>       kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);
>>       switch (cmd) {
> 
> thanks,

Thanks,
Hangyu

Re: [PATCH] tty: vt: add a bounds checking in vt_do_kdgkb_ioctl()

[Dan Carpenter]

On Thu, Sep 08, 2022 at 03:54:03PM +0800, Hangyu Hua wrote:
> As array_index_nospec's comments indicate，a bounds checking need to add
> before calling array_index_nospec.
> 

This commit message doesn't explain the impact to the user so the patch
was going to be rejected based on just the commit message regardless of
the actual code.

regards,
dan carpenter