Thank you for your contribution! Sashiko AI review found 2 potential issue(s) 
to consider:
- [High] get_offset_of_field() incorrectly evaluates the BTF kflag of the 
parent struct instead of the nested anonymous struct containing the field, 
leading to massively inflated offset calculations and bypassing bitfield 
validation.
- [Low] wprobe_trigger_log_err receives a truncated `glob` string, causing 
syntax error logs to place the caret out-of-bounds and hide the user's input.
--

--- Patch [12]: [PATCH v13 12/12] tracing/wprobe: Support BTF struct offset 
resolution in set_wprobe trigger ---
commit 1da976d66d95af47e154412d3c1b912f8a6b4018
Author: Masami Hiramatsu (Google) <[email protected]>

tracing/wprobe: Support BTF struct offset resolution in set_wprobe trigger
    
Allow set_wprobe trigger to use BTF struct offset resolution to specify
the target address field.

Link: 
https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao...@gmail.com/

> diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> index acb15fd018e80..b513561005dc6 100644
> --- a/kernel/trace/trace_wprobe.c
> +++ b/kernel/trace/trace_wprobe.c

[ ... ]

> @@ -990,6 +992,181 @@ static void wprobe_trigger_free(struct 
> event_trigger_data *data)
>       }
>  }
>  
> +#ifdef CONFIG_PROBE_EVENTS_BTF_ARGS
> +
> +static int get_offset_of_field(struct btf *btf, const struct btf_type *type, 
> char *field_name)
> +{
> +     const struct btf_member *field;
> +     int bitoffs = 0;
> +     u32 anon_offs;
> +     char *next;
> +
> +     do {
> +             next = strchr(field_name, '.');
> +             if (next)
> +                     *next++ = '\0';
> +
> +             field = btf_find_struct_member(btf, type, field_name, 
> &anon_offs);
> +             if (IS_ERR_OR_NULL(field))
> +                     return -ENOENT;
> +
> +             if (btf_type_kflag(type)) {

[Severity: High]
Does this code check the kflag of the original parent type instead of the
anonymous struct that actually contains the field?

btf_find_struct_member() can return a field from a nested anonymous struct,
but if the parent struct and the anonymous struct have mismatched BTF kflags
(for example, the parent has kflag=0 while the anonymous struct has kflag=1
because it contains bitfields), using the parent's kflag here will cause
the bitfield size encoded in the upper 8 bits of the offset to be
misinterpreted as part of the byte offset.

This can add a massively inflated value to bitoffs and result in a wild
pointer dereference when the wprobe triggers.

> +                     /* Reject bitfield member access */
> +                     if (BTF_MEMBER_BITFIELD_SIZE(field->offset))
> +                             return -EINVAL;
> +                     bitoffs += anon_offs + 
> BTF_MEMBER_BIT_OFFSET(field->offset);
> +             } else {
> +                     bitoffs += anon_offs + field->offset;
> +             }
> +
> +             field_name = next;
> +             if (next) {
> +                     type = btf_type_skip_modifiers(btf, field->type, NULL);
> +                     if (!type)
> +                             return -ENOENT;
> +             }
> +     } while (next);
> +     return bitoffs / BITS_PER_BYTE;
> +}

[ ... ]

> +static int wprobe_trigger_typecast_parse(char **field_str_ptr,
> +                                      struct trace_event_file *file,
> +                                      struct wprobe_trigger_data 
> *wprobe_data,
> +                                      const char *glob)
> +{
> +     struct btf *btf __free(btf_put) = NULL;
> +     const struct btf_type *type;
> +     char *assign_field;
> +     char *event_field;
> +     char *type_field;
> +     char *type_name;
> +     char *offs;
> +     long val = 0;
> +     int id;
> +     int adjust;
> +
> +     type_name = *field_str_ptr + 1;
> +     event_field = strchr(type_name, ')');
> +     if (!event_field) {
> +             wprobe_trigger_log_err(file, glob, type_name - glob, 
> DEREF_OPEN_BRACE);

[Severity: Low]
Will this log message place the caret out of bounds and hide the user's
input?

The glob string passed into wprobe_trigger_log_err() seems to be truncated
by an earlier strsep() call replacing the delimiter with a null terminator:

kernel/trace/trace_events_trigger.c:trigger_process_regex() {
    ...
    command = strsep(&next, ": \t");
    ...
}

Since glob is truncated to just the command name, calculating the offset as
type_name - glob can point far past the end of the printed text, making the
syntax error logs illegible.

> +             return -EINVAL;
> +     }

-- 
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/178739053919.1520941.17662338993878200834.stgit@devnote2?part=12

Reply via email to