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

  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>
---
v5 -> v6:
- Ensure that the following extensions remain effective:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-uevent

v4 -> v5:
- Fixed build errors and warnings, and retested the patch.

v3 -> v4:
- Refactor the function to be more obviously correct and readable.
---
 include/linux/kobject.h |  3 +++
 lib/kobject_uevent.c    | 30 +++++++++++++++++-------------
 2 files changed, 20 insertions(+), 13 deletions(-)
  

On Wed, May 17, 2023 at 06:19:57PM +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>
> ---
> v5 -> v6:
> - Ensure that the following extensions remain effective:
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-uevent
> 
> v4 -> v5:
> - Fixed build errors and warnings, and retested the patch.
> 
> v3 -> v4:
> - Refactor the function to be more obviously correct and readable.
> ---
>  include/linux/kobject.h |  3 +++
>  lib/kobject_uevent.c    | 30 +++++++++++++++++-------------
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index c392c811d9ad..9d3ecce3c4f6 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -32,6 +32,9 @@
>  #define UEVENT_NUM_ENVP			64	/* number of env pointers */
>  #define UEVENT_BUFFER_SIZE		2048	/* buffer for the variables */
>  
> +/* the maximum length of the string contained in kobject_actions[] */
> +#define UEVENT_KACT_STRSIZE		16

Why does this value need to be in a global .h file when it is only used
in one .c file?

And how are you going to keep it in sync with kobject_actions if it
changes in the future?  And that variable isn't even in this file, how
would anyone know to modify this if the structure changes in a .c file?

> +
>  #ifdef CONFIG_UEVENT_HELPER
>  /* path to the userspace helper executed on an event */
>  extern char uevent_helper[];
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 7c44b7ae4c5c..4030a928e9c6 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -66,7 +66,8 @@ 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;
> +	char kobj_act_buf[UEVENT_KACT_STRSIZE] = "";

Why does this need to be initialized?

And are you sure the size is correct?  If so, how?

And how was any of this tested?  Based on your prior submissions, we are
going to require some sort of proof.  What would you do if you were in
my position?

thanks,

greg k-h
  

On 2023/5/17 20:17, Greg KH wrote:
> On Wed, May 17, 2023 at 06:19:57PM +0800, Xia Fukun wrote:
>> --- a/include/linux/kobject.h
>> +++ b/include/linux/kobject.h
>> @@ -32,6 +32,9 @@
>>  #define UEVENT_NUM_ENVP			64	/* number of env pointers */
>>  #define UEVENT_BUFFER_SIZE		2048	/* buffer for the variables */
>>  
>> +/* the maximum length of the string contained in kobject_actions[] */
>> +#define UEVENT_KACT_STRSIZE		16
> 
> Why does this value need to be in a global .h file when it is only used
> in one .c file?
> 
> And how are you going to keep it in sync with kobject_actions if it
> changes in the future?  And that variable isn't even in this file, how
> would anyone know to modify this if the structure changes in a .c file?


Your criticism is correct. UEVENT_KACT_STRSIZE should not be defined
in the global .h file here. I will move it to that .c file.


>> --- a/lib/kobject_uevent.c
>> +++ b/lib/kobject_uevent.c
>> @@ -66,7 +66,8 @@ 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;
>> +	char kobj_act_buf[UEVENT_KACT_STRSIZE] = "";
> 
> Why does this need to be initialized?


My initialization method has some flaws, which should be done as follows:

char kobj_act_buf[UEVENT_KACT_STRSIZE] = {0};

Initialize the string kobj_act_buf to "/0" and parse it
using sysfs_match_string after subsequent copy operations.


> And are you sure the size is correct?  If so, how?

UEVENT_KACT_STRSIZE is defined as the maximum length of the string
contained in kobject_actions[].

At present, the maximum length of strings in this array is 7. Based on
the actual meaning of these strings, these actions will not exceed 16
if there are any subsequent changes.

> And how was any of this tested?  Based on your prior submissions, we are
> going to require some sort of proof.  What would you do if you were in
> my position?

My testing method is to apply the patch, compile the kernel image,
and start the QEMU virtual machine. Then compile and execute the code
mentioned in the patch that triggers out-of-bounds issues.

In addition, the following operations will be performed to verify the
functions mentioned by Peter Rajnoha <prajnoha@redhat.com>:

# echo "add fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed A=1 B=abc" >
/sys/block/ram0/uevent

# udevadm monitor --kernel --env
monitor will print the received events for:
KERNEL - the kernel uevent

KERNEL[189.376386] add      /devices/virtual/block/ram0 (block)
ACTION=add
DEVPATH=/devices/virtual/block/ram0
SUBSYSTEM=block
SYNTH_UUID=fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed
SYNTH_ARG_A=1
SYNTH_ARG_B=abc
DEVNAME=/dev/ram0
DEVTYPE=disk
DISKSEQ=14
SEQNUM=3781
MAJOR=1
MINOR=0

> thanks,
> 
> greg k-h

Thank you for your suggestion. My submission was indeed negligent,
and your guidance has benefited me greatly.
  

Gentle ping ...

On 2023/5/18 10:37, Xia Fukun wrote:
> On 2023/5/17 20:17, Greg KH wrote:
> 
>> And how was any of this tested?  Based on your prior submissions, we are
>> going to require some sort of proof.  What would you do if you were in
>> my position?
> 
> My testing method is to apply the patch, compile the kernel image,
> and start the QEMU virtual machine. Then compile and execute the code
> mentioned in the patch that triggers out-of-bounds issues.
> 
> In addition, the following operations will be performed to verify the
> functions mentioned by Peter Rajnoha <prajnoha@redhat.com>:
> 
> # echo "add fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed A=1 B=abc" >
> /sys/block/ram0/uevent
> 
> # udevadm monitor --kernel --env
> monitor will print the received events for:
> KERNEL - the kernel uevent
> 
> KERNEL[189.376386] add      /devices/virtual/block/ram0 (block)
> ACTION=add
> DEVPATH=/devices/virtual/block/ram0
> SUBSYSTEM=block
> SYNTH_UUID=fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed
> SYNTH_ARG_A=1
> SYNTH_ARG_B=abc
> DEVNAME=/dev/ram0
> DEVTYPE=disk
> DISKSEQ=14
> SEQNUM=3781
> MAJOR=1
> MINOR=0
> 
> Thank you for your suggestion. My submission was indeed negligent,
> and your guidance has benefited me greatly.

I have submitted v7 of the patch according to your suggestion and
tested it to ensure its functionality is correct.

Please take the time to review it.

Thank you very much.
  

On Tue, May 23, 2023 at 04:52:23PM +0800, Xia Fukun wrote:
> Gentle ping ...

Please relax, there are lots of other changes to review before yours,
and frankly, due to all of the problems that this patch has had over
time, it's on the bottom of my list.

To help out, why don't you review stuff as well?

thanks,

greg k-h