Re: [PATCH] bpf: explicitly memset the bpf_attr structure

[Daniel Borkmann <daniel@iogearbox.net>]

On 3/20/20 4:45 PM, Greg Kroah-Hartman wrote:
> On Fri, Mar 20, 2020 at 04:24:32PM +0100, Daniel Borkmann wrote:
>> On 3/20/20 10:48 AM, Greg Kroah-Hartman wrote:
>>> For the bpf syscall, we are relying on the compiler to properly zero out
>>> the bpf_attr union that we copy userspace data into.  Unfortunately that
>>> doesn't always work properly, padding and other oddities might not be
>>> correctly zeroed, and in some tests odd things have been found when the
>>> stack is pre-initialized to other values.
>>>
>>> Fix this by explicitly memsetting the structure to 0 before using it.
>>>
>>> Reported-by: Maciej Żenczykowski <maze@google.com>
>>> Reported-by: John Stultz <john.stultz@linaro.org>
>>> Reported-by: Alexander Potapenko <glider@google.com>
>>> Reported-by: Alistair Delva <adelva@google.com>
>>> Cc: stable <stable@vger.kernel.org>
>>> Link: https://android-review.googlesource.com/c/kernel/common/+/1235490
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>>    kernel/bpf/syscall.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index a91ad518c050..a4b1de8ea409 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -3354,7 +3354,7 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
>>>    SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
>>>    {
>>> -	union bpf_attr attr = {};
>>> +	union bpf_attr attr;
>>>    	int err;
>>>    	if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
>>> @@ -3366,6 +3366,7 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
>>>    	size = min_t(u32, size, sizeof(attr));
>>>    	/* copy attributes from user space, may be less than sizeof(bpf_attr) */
>>> +	memset(&attr, 0, sizeof(attr));
>>
>> Thanks for the fix, there are a few more of these places. We would also need
>> to cover:
>>
>> - bpf_prog_get_info_by_fd()
> 
> Unless I am mistaken, struct bpf_prog_info is packed fully, with no
> holes, so this shouldn't be an issue there.

It does have a '/* XXX 31 bits hole, try to pack */' but I presume the compiler
might simply zero it in this case.

>> - bpf_map_get_info_by_fd()
> 
> No padding in struct bpf_map_info that I can see, so I doubt this is
> needed there.
> 
>> - btf_get_info_by_fd()
> 
> There is no padding in struct bpf_btf_info, so that's not needed there,
> but I can add it if you really want.
> 
> I can change these, but I don't think that there currently is a bug in
> those functions, unlike with "union bpf_attr" which, as Yonghong points
> out, is tripping on the CHECK_ATTR() test later on.

Got it, my main concern is that the next time someone extends these fields with
new members we could potentially add holes in there as well and we'll run into
the same issue twice, example from the past is b85fab0e67b1 ("bpf: Add gpl_compatible
flag to struct bpf_prog_info").

Thanks,
Daniel