Re: [PATCH 2/2] tracing/user_events: reject overflowing field offsets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tracing/user_events: u32 struct size wrap in user_event_parse_field()
--

commit 3b2945a002d22d5a8a1ae3cae8f4c9ec6c460511
Author: Yifan Zhang <yfzhang23@stu.pku.edu.cn>

tracing/user_events: reject overflowing field offsets

This patch adds an overflow check in user_event_parse_field() to reject field
offsets that overflow a u32 or exceed INT_MAX when registering a user event.

> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 85938b45b7618..88d384c9e101f 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -1207,6 +1207,7 @@ static int user_event_parse_field(char *field, struct user_event *user,
>  {
>  	char *part, *type, *name;
>  	u32 depth = 0, saved_offset = *offset;
> +	u32 new_offset;
>  	int len, size = -EINVAL;
>  	bool is_struct = false;
>  
> @@ -1287,7 +1288,11 @@ static int user_event_parse_field(char *field, struct user_event *user,
>  	if (size < 0)
>  		return size;
>  
> -	*offset = saved_offset + size;
> +	if (check_add_overflow(saved_offset, (u32)size, &new_offset) ||
> +	    new_offset > INT_MAX)
> +		return -E2BIG;

[Severity: High]
What happens when a user supplies a struct field size between INT_MAX + 1 and
U32_MAX?

Earlier in user_event_parse_field(), the struct size is parsed into the signed
int variable size via kstrtou32():

	case FIELD_DEPTH_SIZE:
		if (!is_struct)
			return -EINVAL;

		if (kstrtou32(part, 10, &size))
			return -EINVAL;
		break;

When part is in [2147483648, 4294967295], kstrtou32() succeeds and writes a
value into size that is negative when interpreted as a signed int. The
preceding check:

	if (size < 0)
		return size;

then returns that user-controlled negative integer directly instead of
reaching the new overflow check.

When reached through user_events_ioctl() -> user_events_ioctl_reg() ->
user_event_parse_cmd() -> user_event_parse() -> user_event_parse_fields() ->
user_event_parse_field(), or through create_dyn_event() ->
user_event_create(), could this return arbitrary negative values (including
kernel-internal restart codes like -ERESTARTSYS, values outside [-MAX_ERRNO,
-1], or -ECANCELED) to the caller? Also, since both saved_offset and size are
bounded to INT_MAX after the size < 0 check, can check_add_overflow() ever
trigger here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179362071771.29269.15129360535376085397@stu.pku.edu.cn?part=2