Re: linux-next: build failure after merge of the kspp tree

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Kees,

On Mon, 24 Jan 2022 23:44:05 -0800
Kees Cook <keescook@chromium.org> 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@kernel.org>