Date:   Mon, 06 Nov 2017 21:25:41 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Alexei Starovoitov <ast@...com>, netdev@...r.kernel.org,
        Sandipan Das <sandipan@...ux.vnet.ibm.com>
Cc:     Brendan Gregg <brendan.d.gregg@...il.com>, daniel@...earbox.net,
        Martin KaFai Lau <kafai@...com>,
        Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] bpf: Add helpers to read useful task_struct members

Alexei Starovoitov wrote:
> On 11/5/17 2:31 AM, Naveen N. Rao wrote:
>> Hi Alexei,
>>
>> Alexei Starovoitov wrote:
>>> On 11/3/17 3:58 PM, Sandipan Das wrote:
>>>> For added security, the layout of some structures can be
>>>> randomized by enabling CONFIG_GCC_PLUGIN_RANDSTRUCT. One
>>>> such structure is task_struct. To build BPF programs, we
>>>> use Clang which does not support this feature. So, if we
>>>> attempt to read a field of a structure with a randomized
>>>> layout within a BPF program, we do not get the expected
>>>> value because of incorrect offsets. To observe this, it
>>>> is not mandatory to have CONFIG_GCC_PLUGIN_RANDSTRUCT
>>>> enabled because the structure annotations/members added
>>>> for this purpose are enough to cause this. So, all kernel
>>>> builds are affected.
>>>>

...

>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index f90860d1f897..324508d27bd2 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -338,6 +338,16 @@ union bpf_attr {
>>>>   *     @skb: pointer to skb
>>>>   *     Return: classid if != 0
>>>>   *
>>>> + * u64 bpf_get_task_pid_tgid(struct task_struct *task)
>>>> + *     Return: task->tgid << 32 | task->pid
>>>> + *
>>>> + * int bpf_get_task_comm(struct task_struct *task)
>>>> + *     Stores task->comm into buf
>>>> + *     Return: 0 on success or negative error
>>>> + *
>>>> + * u32 bpf_get_task_flags(struct task_struct *task)
>>>> + *     Return: task->flags
>>>> + *
>>>
>>> I don't think it's a solution.
>>> Tracing scripts read other fields too.
>>> Making it work for these 3 fields is a drop in a bucket.
>>
>> Indeed. However...
>>
>>> If randomization is used I think we have to accept
>>> that existing bpf scripts won't be usable.
>>
>> ... the actual issue is that randomization isn't necessary for this to
>> show up. The annotations added to mark off the structure members results
>> in some structure members being moved into an anonymous structure, which
>> would then get padded differently. So, *all* kernels since v4.13 are
>> affected, afaict.
> 
> hmm. why would all 4.13+ be affected?
> It's just an anonymous struct inside task_struct.
> Are you saying that due to clang not adding this 'struct { };' treatment 
> to task_struct?

Yes, that's what it looked like.

> I thought such struct shouldn't change layout.
> If it is we need to fix include/linux/compiler-clang.h to do that
> anon struct as well.

We considered that, but it looked to be very dependent on the version of 
gcc used to build the kernel. But, this may be a simpler approach for 
the shorter term.

> 
>> As such, we wanted to propose this as a short term solution, but I do
>> agree that this doesn't solve the real issue.
>>
>>> Long term solution is to support 'BPF Type Format' or BTF
>>> (which is old C-Type Format) for kernel data structures,
>>> so bcc scripts wouldn't need to use kernel headers and clang.
>>> The proper offsets will be described in BTF.
>>> We were planning to use it initially to describe map key/value,
>>> but it applies for this case as well.
>>> There will be a tool that will take dwarf from vmlinux and
>>> compress it into BTF. Kernel will also be able to verify
>>> that BTF is a valid BTF.
>>
>> This is the first that I've heard about BTF. Can you share more details
>> about it, or point me to some place where it has been discussed?
>>
>> We considered having tools derive the structure offsets from debuginfo,
>> but debuginfo may not always be present on production systems. So, it
>> isn't clear if having that dependency is fine. I'm not sure how BTF will
>> be different.
> 
> It was discussed at this year plumbers:
> https://lwn.net/Articles/734453/
> 
> btw the name BTF is work in progress. We started with CTF, but
> it conflicts with all other meanings of this abbreviation.
> Likely we will call it something different at the end.
> 
> Initial goal was to describe key/map values of bpf maps to
> make debugging easier, but now we want to use this compressed
> type format for tracing as well, since installing kernel headers
> everywhere doesn't scale well while CTF can be embedded in vmlinux

Makes sense, though I'm curious on how you're planning to have this work
without the kernel headers :)

> 
> We were also thinking to improve verifier with CTF knowledge too.
> Like if CTF describes that map value is two u32, but bpf program
> is doing 8-byte access then something is wrong and either warn
> or reject such program.

Sounds good. I look forward to more details/patches on this front once 
you're ready to share more.

Thanks,
- Naveen

Powered by blists - more mailing lists