diff mbox series

[v4] kobject: Fix global-out-of-bounds in kobject_action_type()

Message ID 20230512103029.183852-1-xiafukun@huawei.com
State New
Headers
Series [v4] kobject: Fix global-out-of-bounds in kobject_action_type() |

Commit Message

Xia Fukun May 12, 2023, 10:30 a.m. UTC 
  The following c language code can trigger KASAN's global variable
out-of-bounds access error in kobject_action_type():

int main() {
    int fd;
    char *filename = "/sys/block/ram12/uevent";
    char str[86] = "offline";
    int len = 86;

    fd = open(filename, O_WRONLY);
    if (fd == -1) {
        printf("open");
        exit(1);
    }

    if (write(fd, str, len) == -1) {
        printf("write");
        exit(1);
    }

    close(fd);
    return 0;
}

Function kobject_action_type() receives the input parameters buf and count,
where count is the length of the string buf.

In the use case we provided, count is 86, the count_first is 85.
Buf points to a string with a length of 86, and its first seven
characters are "offline".
In line 87 of the code, kobject_actions[action] is the string "offline"
with the length of 7,an out-of-boundary access will appear:

kobject_actions[action][85].

Use sysfs_match_string() to replace the fragile and convoluted loop.
This function is well-tested for parsing sysfs inputs. Moreover, this
modification will not cause any functional changes.

Fixes: f36776fafbaa ("kobject: support passing in variables for synthetic uevents")
Signed-off-by: Xia Fukun <xiafukun@huawei.com>
---
v3 -> v4:
- refactor the function to be more obviously correct and readable

v2 -> v3:
- only declare that it is the latest version of the patch, no change

v1 -> v2:
- modify the matching logic

 lib/kobject_uevent.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

kernel test robot May 12, 2023, 4:20 p.m. UTC | #1 
Hi Xia,

kernel test robot noticed the following build errors:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.4-rc1 next-20230512]
[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/Xia-Fukun/kobject-Fix-global-out-of-bounds-in-kobject_action_type/20230512-183414
base:   driver-core/driver-core-testing
patch link:    https://lore.kernel.org/r/20230512103029.183852-1-xiafukun%40huawei.com
patch subject: [PATCH v4] kobject: Fix global-out-of-bounds in kobject_action_type()
config: x86_64-randconfig-a001 (https://download.01.org/0day-ci/archive/20230513/202305130013.em3umwVP-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/4d1d5546395a3bf45324f25b5e77b90fe6cfe8df
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Xia-Fukun/kobject-Fix-global-out-of-bounds-in-kobject_action_type/20230512-183414
        git checkout 4d1d5546395a3bf45324f25b5e77b90fe6cfe8df
        # 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 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305130013.em3umwVP-lkp@intel.com/

All errors (new ones prefixed by >>):

>> lib/kobject_uevent.c:90:11: error: implicit declaration of function 'kobject_action' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           action = kobject_action(i);
                    ^
   lib/kobject_uevent.c:67:9: warning: variable 'count_first' set but not used [-Wunused-but-set-variable]
           size_t count_first;
                  ^
   1 warning and 1 error generated.


vim +/kobject_action +90 lib/kobject_uevent.c

    61	
    62	static int kobject_action_type(const char *buf, size_t count,
    63				       enum kobject_action *type,
    64				       const char **args)
    65	{
    66		enum kobject_action action;
    67		size_t count_first;
    68		const char *args_start;
    69		int i, ret = -EINVAL;
    70	
    71		if (count && (buf[count-1] == '\n' || buf[count-1] == '\0'))
    72			count--;
    73	
    74		if (!count)
    75			goto out;
    76	
    77		args_start = strnchr(buf, count, ' ');
    78		if (args_start) {
    79			count_first = args_start - buf;
    80			args_start = args_start + 1;
    81		} else
    82			count_first = count;
    83	
    84		/* Use sysfs_match_string() to replace the fragile and convoluted loop */
    85		i = sysfs_match_string(kobject_actions, buf);
    86	
    87		if (i < 0)
    88			return ret;
    89	
  > 90		action = kobject_action(i);
    91	
    92		if (args)
    93			*args = args_start;
    94	
    95		*type = action;
    96		ret = 0;
    97	
    98	out:
    99		return ret;
   100	}
   101
kernel test robot May 12, 2023, 4:51 p.m. UTC | #2 
Hi Xia,

kernel test robot noticed the following build errors:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.4-rc1 next-20230512]
[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/Xia-Fukun/kobject-Fix-global-out-of-bounds-in-kobject_action_type/20230512-183414
base:   driver-core/driver-core-testing
patch link:    https://lore.kernel.org/r/20230512103029.183852-1-xiafukun%40huawei.com
patch subject: [PATCH v4] kobject: Fix global-out-of-bounds in kobject_action_type()
config: hexagon-buildonly-randconfig-r004-20230510 (https://download.01.org/0day-ci/archive/20230513/202305130029.EtJLG9kz-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
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/4d1d5546395a3bf45324f25b5e77b90fe6cfe8df
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Xia-Fukun/kobject-Fix-global-out-of-bounds-in-kobject_action_type/20230512-183414
        git checkout 4d1d5546395a3bf45324f25b5e77b90fe6cfe8df
        # 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=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305130029.EtJLG9kz-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from lib/kobject_uevent.c:23:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from lib/kobject_uevent.c:23:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from lib/kobject_uevent.c:23:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> lib/kobject_uevent.c:90:11: error: call to undeclared function 'kobject_action'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           action = kobject_action(i);
                    ^
   lib/kobject_uevent.c:67:9: warning: variable 'count_first' set but not used [-Wunused-but-set-variable]
           size_t count_first;
                  ^
   7 warnings and 1 error generated.


vim +/kobject_action +90 lib/kobject_uevent.c

    61	
    62	static int kobject_action_type(const char *buf, size_t count,
    63				       enum kobject_action *type,
    64				       const char **args)
    65	{
    66		enum kobject_action action;
    67		size_t count_first;
    68		const char *args_start;
    69		int i, ret = -EINVAL;
    70	
    71		if (count && (buf[count-1] == '\n' || buf[count-1] == '\0'))
    72			count--;
    73	
    74		if (!count)
    75			goto out;
    76	
    77		args_start = strnchr(buf, count, ' ');
    78		if (args_start) {
    79			count_first = args_start - buf;
    80			args_start = args_start + 1;
    81		} else
    82			count_first = count;
    83	
    84		/* Use sysfs_match_string() to replace the fragile and convoluted loop */
    85		i = sysfs_match_string(kobject_actions, buf);
    86	
    87		if (i < 0)
    88			return ret;
    89	
  > 90		action = kobject_action(i);
    91	
    92		if (args)
    93			*args = args_start;
    94	
    95		*type = action;
    96		ret = 0;
    97	
    98	out:
    99		return ret;
   100	}
   101
Greg KH May 13, 2023, 9:33 a.m. UTC | #3 
On Fri, May 12, 2023 at 06:30:29PM +0800, Xia Fukun wrote:
> The following c language code can trigger KASAN's global variable
> out-of-bounds access error in kobject_action_type():
> 
> int main() {
>     int fd;
>     char *filename = "/sys/block/ram12/uevent";
>     char str[86] = "offline";
>     int len = 86;
> 
>     fd = open(filename, O_WRONLY);
>     if (fd == -1) {
>         printf("open");
>         exit(1);
>     }
> 
>     if (write(fd, str, len) == -1) {
>         printf("write");
>         exit(1);
>     }
> 
>     close(fd);
>     return 0;
> }
> 
> Function kobject_action_type() receives the input parameters buf and count,
> where count is the length of the string buf.
> 
> In the use case we provided, count is 86, the count_first is 85.
> Buf points to a string with a length of 86, and its first seven
> characters are "offline".
> In line 87 of the code, kobject_actions[action] is the string "offline"
> with the length of 7,an out-of-boundary access will appear:
> 
> kobject_actions[action][85].
> 
> Use sysfs_match_string() to replace the fragile and convoluted loop.
> This function is well-tested for parsing sysfs inputs. Moreover, this
> modification will not cause any functional changes.
> 
> Fixes: f36776fafbaa ("kobject: support passing in variables for synthetic uevents")
> Signed-off-by: Xia Fukun <xiafukun@huawei.com>
> ---
> v3 -> v4:
> - refactor the function to be more obviously correct and readable

How did you test this as it does not even build?

confused,

greg k-h
Xia Fukun May 16, 2023, 1:37 a.m. UTC | #4 
On 2023/5/13 17:33, Greg KH wrote:

> 
> How did you test this as it does not even build?
> 
> confused,
> 
> greg k-h

I am very sorry for my negligence.

I will fix the build error and conduct thorough testing before submitting the next version of the patch.
diff mbox series

Patch

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 7c44b7ae4c5c..59f835f5f870 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -66,7 +66,7 @@  static int kobject_action_type(const char *buf, size_t count,
 	enum kobject_action action;
 	size_t count_first;
 	const char *args_start;
-	int ret = -EINVAL;
+	int i, ret = -EINVAL;
 
 	if (count && (buf[count-1] == '\n' || buf[count-1] == '\0'))
 		count--;
@@ -81,17 +81,20 @@  static int kobject_action_type(const char *buf, size_t count,
 	} else
 		count_first = count;
 
-	for (action = 0; action < ARRAY_SIZE(kobject_actions); action++) {
-		if (strncmp(kobject_actions[action], buf, count_first) != 0)
-			continue;
-		if (kobject_actions[action][count_first] != '\0')
-			continue;
-		if (args)
-			*args = args_start;
-		*type = action;
-		ret = 0;
-		break;
-	}
+	/* Use sysfs_match_string() to replace the fragile and convoluted loop */
+	i = sysfs_match_string(kobject_actions, buf);
+
+	if (i < 0)
+		return ret;
+
+	action = kobject_action(i);
+
+	if (args)
+		*args = args_start;
+
+	*type = action;
+	ret = 0;
+
 out:
 	return ret;
 }