Re: Issue accessing task_struct from BPF due to 4.16 stack-protector changes

[Kees Cook]

On Fri, Mar 2, 2018 at 2:26 PM, Alexei Starovoitov
 wrote:
> On Fri, Mar 02, 2018 at 02:04:17PM -0800, Gianluca Borello wrote:
>> On Fri, Mar 2, 2018 at 12:42 PM, Alexei Starovoitov
>>  wrote:
>> >
>> > good catch!
>> > I wonder why sched.h is using this flag insead of relying on #defines from 
>> > autoconf.h
>> > It could have been using CONFIG_HAVE_CC_STACKPROTECTOR
>> > instead of CONFIG_CC_STACKPROTECTOR, no ?
>> >
>>
>> Thanks for your reply Alexei. I think switching to
>> HAVE_CC_STACKPROTECTOR could indeed solve this particular BPF issue in
>> a cleaner way (I tested it), at the cost of having that struct member
>> always present for the supported architectures even if the stack
>> protector is actually disabled (e.g. CONFIG_CC_STACKPROTECTOR_NONE=y).
>
> if defined(HAVE_CC_STACKPROTECTOR) && !defined(CONFIG_CC_STACKPROTECTOR_NONE)

CONFIG_CC_STACKPROTECTOR_AUTO may result in no stack protector, so
CONFIG_CC_STACKPROTECTOR is the way to determine if it should exist.

> let's fix it properly instead of adding more hacks to Makefiles

It is being fixed properly -- the detection code is being moved out of
Makefile into Kconfig, at which point this won't be as weird as it is.

If KBUILD_CPPFLAGS won't work for you, I'm not hugely opposed to
switching the task_struct ifdef to HAVE_CC_STACKPROTECTOR, since it is
extremely rare to build without stack protector on architectures that
support it.

-Kees

-- 
Kees Cook
Pixel Security

Re: Issue accessing task_struct from BPF due to 4.16 stack-protector changes

[Alexei Starovoitov]

On Fri, Mar 02, 2018 at 02:04:17PM -0800, Gianluca Borello wrote:
> On Fri, Mar 2, 2018 at 12:42 PM, Alexei Starovoitov
>  wrote:
> >
> > good catch!
> > I wonder why sched.h is using this flag insead of relying on #defines from 
> > autoconf.h
> > It could have been using CONFIG_HAVE_CC_STACKPROTECTOR
> > instead of CONFIG_CC_STACKPROTECTOR, no ?
> >
> 
> Thanks for your reply Alexei. I think switching to
> HAVE_CC_STACKPROTECTOR could indeed solve this particular BPF issue in
> a cleaner way (I tested it), at the cost of having that struct member
> always present for the supported architectures even if the stack
> protector is actually disabled (e.g. CONFIG_CC_STACKPROTECTOR_NONE=y).

if defined(HAVE_CC_STACKPROTECTOR) && !defined(CONFIG_CC_STACKPROTECTOR_NONE)

or

def(have_cc) && (def(cc_stack_regular) || def(cc_stack_strong) || 
def(cc_stack_auto))

let's fix it properly instead of adding more hacks to Makefiles

Re: Issue accessing task_struct from BPF due to 4.16 stack-protector changes

[Kees Cook]

On Fri, Mar 2, 2018 at 2:04 PM, Gianluca Borello  wrote:
> On Fri, Mar 2, 2018 at 12:42 PM, Alexei Starovoitov
>  wrote:
>>
>> good catch!
>> I wonder why sched.h is using this flag insead of relying on #defines from 
>> autoconf.h
>> It could have been using CONFIG_HAVE_CC_STACKPROTECTOR
>> instead of CONFIG_CC_STACKPROTECTOR, no ?
>>
>
> Thanks for your reply Alexei. I think switching to
> HAVE_CC_STACKPROTECTOR could indeed solve this particular BPF issue in
> a cleaner way (I tested it), at the cost of having that struct member
> always present for the supported architectures even if the stack
> protector is actually disabled (e.g. CONFIG_CC_STACKPROTECTOR_NONE=y).
>
> Not sure if this could be frowned upon by someone considering how
> critical task_struct is, but on the other hand is really just 8 bytes.

That structure is huge, and I think it's proper to leave this as is.

Adding KBUILD_CPPFLAGS (for now) seems like the right way to go;
though in the future stack protector will be changed around again (to
be purely Kconfig again). There are a number of issues with its logic
in detecting and enabling, and another draft at solving it is under
development.

-Kees

-- 
Kees Cook
Pixel Security

Re: Issue accessing task_struct from BPF due to 4.16 stack-protector changes

[Gianluca Borello]

On Fri, Mar 2, 2018 at 12:42 PM, Alexei Starovoitov
 wrote:
>
> good catch!
> I wonder why sched.h is using this flag insead of relying on #defines from 
> autoconf.h
> It could have been using CONFIG_HAVE_CC_STACKPROTECTOR
> instead of CONFIG_CC_STACKPROTECTOR, no ?
>

Thanks for your reply Alexei. I think switching to
HAVE_CC_STACKPROTECTOR could indeed solve this particular BPF issue in
a cleaner way (I tested it), at the cost of having that struct member
always present for the supported architectures even if the stack
protector is actually disabled (e.g. CONFIG_CC_STACKPROTECTOR_NONE=y).

Not sure if this could be frowned upon by someone considering how
critical task_struct is, but on the other hand is really just 8 bytes.

Thanks

Re: Issue accessing task_struct from BPF due to 4.16 stack-protector changes

[Alexei Starovoitov]

On Fri, Mar 02, 2018 at 12:09:57PM -0800, Gianluca Borello wrote:
> Hello,
> 
> While testing bpf-next, I noticed that I was reading garbage when
> accessing some task_struct members, and the issue seems caused by the
> recent commit 2bc2f688fdf8 ("Makefile: move stack-protector
> availability out of Kconfig") which removes CONFIG_CC_STACKPROTECTOR
> from autoconf.h.
> 
> When I compile my BPF program, offsetof(struct task_struct, files),
> which is the member I'm dereferencing, returns 1768 (where the garbage
> is), whereas doing it on 4.15 returns 1776 (where the correct member
> is). I believe when compiling with clang this portion of the
> task_struct doesn't get considered anymore:
> 
> #ifdef CONFIG_CC_STACKPROTECTOR
> /* Canary value for the -fstack-protector GCC feature: */
> unsigned long stack_canary;
> #endif
> 
> I solved it by adding $(KBUILD_CPPFLAGS) to my BPF Makefile (which is
> pretty similar to the one used in samples/bpf/Makefile).
> 
> Two questions:
> 
> 1) Do you confirm this is the proper way to handle this moving
> forward? Or should there be a better way?
> 
> 2) Would you consider useful a simple patch to samples/bpf/Makefile so
> that other developers will not be stuck in a long bisect to figure out
> why they read garbage when dereferencing task_struct? I assume that
> several people use that Makefile as a template to start their project,
> like I did (perhaps I'm assuming wrong though).

good catch!
I wonder why sched.h is using this flag insead of relying on #defines from 
autoconf.h
It could have been using CONFIG_HAVE_CC_STACKPROTECTOR
instead of CONFIG_CC_STACKPROTECTOR, no ?