Re: [PATCH v7] kobject: Fix global-out-of-bounds in kobject_action_type()

[Xia Fukun]

On 2023/5/18 17:16, Xia Fukun wrote:
> ---
> v6 -> v7:
> -  Move macro UEVENT_KACT_STRSIZE to the .c file to 
> improve maintainability.
> 

Gentle ping ...

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.

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.


> ---
>  lib/kobject_uevent.c | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 7c44b7ae4c5c..2171e1648dad 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -47,6 +47,14 @@ static LIST_HEAD(uevent_sock_list);
>  /* This lock protects uevent_seqnum and uevent_sock_list */
>  static DEFINE_MUTEX(uevent_sock_mutex);
>  
> +/*
> + * The maximum length of the string contained in kobject_actions[].
> + * If there are any actions added or modified, please ensure that
> + * the string length does not exceed the macro, otherwise
> + * should modify the macro definition.
> + */
> +#define UEVENT_KACT_STRSIZE		16
> +
>  /* the strings here must match the enum in include/linux/kobject.h */
>  static const char *kobject_actions[] = {
>  	[KOBJ_ADD] =		"add",
> @@ -66,7 +74,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] = {0};
>  
>  	if (count && (buf[count-1] == '\n' || buf[count-1] == '\0'))
>  		count--;
> @@ -77,21 +86,24 @@ static int kobject_action_type(const char *buf, size_t count,
>  	args_start = strnchr(buf, count, ' ');
>  	if (args_start) {
>  		count_first = args_start - buf;
> +		if (count_first > UEVENT_KACT_STRSIZE)
> +			goto out;
> +
>  		args_start = args_start + 1;
> +		strncpy(kobj_act_buf, buf, count_first);
> +		i = sysfs_match_string(kobject_actions, kobj_act_buf);
>  	} else
> -		count_first = count;
> +		i = sysfs_match_string(kobject_actions, buf);
>  
> -	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;
> -	}
> +	if (i < 0)
> +		goto out;
> +
> +	action = i;
> +	if (args)
> +		*args = args_start;
> +
> +	*type = action;
> +	ret = 0;
>  out:
>  	return ret;
>  }