As array_index_nospec's comments indicate，a bounds checking need to add
before calling array_index_nospec.
Signed-off-by: Hangyu Hua <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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/[email protected]/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 <[email protected]>
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
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 <[email protected]>
>> ---
>> 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
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