Hi Kees,
On Mon, 24 Jan 2022 23:44:05 -0800
Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Tue, Jan 25, 2022 at 02:50:06PM +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the kspp tree, today's linux-next build (powerpc
> > allmodconfig) failed like this:
> >
> > In file included from include/linux/string.h:253,
> > from include/linux/bitmap.h:11,
> > from include/linux/cpumask.h:12,
> > from include/linux/mm_types_task.h:14,
> > from include/linux/mm_types.h:5,
> > from include/linux/buildid.h:5,
> > from include/linux/module.h:14,
> > from samples/trace_events/trace-events-sample.c:2:
> > In function '__fortify_strcpy',
> > inlined from 'perf_trace_foo_rel_loc' at samples/trace_events/./trace-events-sample.h:519:1:
> > include/linux/fortify-string.h:47:33: error: '__builtin_strcpy' offset 12 is out of the bounds [0, 4] [-Werror=array-bounds]
>
> -Warray-bounds thinks something is trying to get at offset 12 of an
> object it thinks is only 4 bytes in size.
>
> > 47 | #define __underlying_strcpy __builtin_strcpy
> > | ^
> > include/linux/fortify-string.h:445:24: note: in expansion of macro '__underlying_strcpy'
> > 445 | return __underlying_strcpy(p, q);
> > | ^~~~~~~~~~~~~~~~~~~
> >
> > Exposed by (probably) commit
> >
> > 602670289b69 ("fortify: Detect struct member overflows in memcpy() at compile-time")
> >
> > Introduced by commit
> >
> > b466b1332164 ("samples/trace_event: Add '__rel_loc' using sample event")
> >
> > I have reverted that latter commit for today.
>
> Digging through the macros, I end up reconstructing this:
>
> strcpy( (char *)((void *)(&__entry->__rel_loc_foo) +
> sizeof(__entry->__rel_loc_foo) +
> (__entry->__rel_loc_foo & 0xffff)),
> foo ? (const char *)(foo) : "(null)");
>
> I couldn't figure out how __entry is being allocated, but it seemed
> maybe related to this note:
The __entry is the trace-event entry on the trace ring_buffer. This
reserved an entry (area) on the ring_buffer and fills it with
given traced data. "__rel_loc_foo" is the a field on the entry,
which type is u32. This should be something like this.
struct {
...
u32 __rel_loc_foo;
...
} *__entry;
>
> /*
> * struct trace_event_data_offsets_<call> {
> * u32 <item1>;
> * u32 <item2>;
> * [...]
> * };
> *
> * The __dynamic_array() macro will create each u32 <item>, this is
> * to keep the offset of each array from the beginning of the event.
> * The size of an array is also encoded, in the higher 16 bits of
> * <item>.
> */
>
> So, I think -Warray-bounds is refusing to see the destination as
> anything except a u32, but being accessed at 4 (sizeof(u32)) + 8
> (address && 0xffff) (?)
Ah, I got it. Yes, that's right. __data_loc() will access the data
from the __entry, but the __rel_loc() points the same address from
the encoded field ("__rel_loc_foo" in this case) itself.
This is introduced for the user application event, which doesn't
know the actual __entry size because the __entry includes some
kernel internal defined fields.
> But if this is true, I would imagine there would be plenty of other
> warnings? I'm currently stumped.
That is because __rel_loc is used only in the sample code in the kernel
for testing. Other use-cases comes from user-space.
Hmm, can we skip this boundary check for this example?
Thank you,
>
> Reading 55de2c0b5610 ("tracing: Add '__rel_loc' using trace event
> macros") did not help me. ;)
>
> -Kees
>
> --
> Kees Cook
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
