[PATCH] bpf: explicitly memset the bpf_attr structure

[PATCH] bpf: explicitly memset the bpf_attr structure

[Greg Kroah-Hartman]

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));
 	if (copy_from_user(&attr, uattr, size) != 0)
 		return -EFAULT;
 

base-commit: 6c90b86a745a446717fdf408c4a8a4631a5e8ee3
-- 
2.25.2

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

[Yonghong Song]

On 3/20/20 2: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.

Maybe add more contexts about the failure itself so it could be clear
why we need this patch.

As far as I know from the link below, the failure happens in
CHECK_ATTR() which checks any unused *area* for a particular subcommand
must be 0, and this patch tries to provide this guarantee beyond
area beyond min(uattr_size, sizeof(attr)).

> 
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__android-2Dreview.googlesource.com_c_kernel_common_-2B_1235490&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=LW31qE_U9SQxf_FnwMUaEXeM9h54NJ1fOf44hk_QDWk&s=HJ-aQi8Ho6V6ZegmWlPYJqnY7e3KRKfFjFj6C2yEN04&e=
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   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));
>   	if (copy_from_user(&attr, uattr, size) != 0)
>   		return -EFAULT;
>   
> 
> base-commit: 6c90b86a745a446717fdf408c4a8a4631a5e8ee3
> 

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

[Greg Kroah-Hartman]

On Fri, Mar 20, 2020 at 08:23:57AM -0700, Yonghong Song wrote:
> 
> 
> On 3/20/20 2: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.
> 
> Maybe add more contexts about the failure itself so it could be clear
> why we need this patch.

I didn't have the full details, I think Maciej has them though.

> As far as I know from the link below, the failure happens in
> CHECK_ATTR() which checks any unused *area* for a particular subcommand
> must be 0, and this patch tries to provide this guarantee beyond
> area beyond min(uattr_size, sizeof(attr)).

That macro also will get tripped up if padding is not zeroed out as
well, so this is good to fix up.

thanks for the review.

greg k-h

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

[Daniel Borkmann]

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()
- bpf_map_get_info_by_fd()
- btf_get_info_by_fd()

Please add these as well to your fix.

>   	if (copy_from_user(&attr, uattr, size) != 0)
>   		return -EFAULT;
>   
> 
> base-commit: 6c90b86a745a446717fdf408c4a8a4631a5e8ee3
> 

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

[Yonghong Song]

On 3/20/20 8:24 AM, 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://urldefense.proofpoint.com/v2/url?u=https-3A__android-2Dreview.googlesource.com_c_kernel_common_-2B_1235490&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=Fz_Xc6psG64uMowK2qpH0gTLj0NQE7k1CTWb5fODVeg&s=WKW0vq8WBALfwsSq5xGGWwxuLWKfI0DNN9XVMc1DkcE&e= 
>> 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()
> - bpf_map_get_info_by_fd()
> - btf_get_info_by_fd()

Not sure whether in these places existing approach will cause
kernel failure or not. They did not call CHECK_ATTR, e.g.,
for bpf_prog_info structure.

> 
> Please add these as well to your fix.
> 
>>       if (copy_from_user(&attr, uattr, size) != 0)
>>           return -EFAULT;
>>
>> base-commit: 6c90b86a745a446717fdf408c4a8a4631a5e8ee3
>>
> 

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

[Greg Kroah-Hartman]

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.

> - 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.

thanks,

greg k-h

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

[Daniel Borkmann]

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

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

[Greg Kroah-Hartman]

On Fri, Mar 20, 2020 at 05:04:24PM +0100, Daniel Borkmann wrote:
> 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").

Fair enough, I'll make a second patch for this, as there's no known
issue today with those initializations that need to be backported to the
stable tree :)

thanks,

greg k-h

[PATCH] bpf: explicitly memset some bpf info structures declared on the stack

[Greg Kroah-Hartman]

Trying to initialize a structure with "= {};" will not always clean out
all padding locations in a structure.  So be explicit and call memset to
initialize everything for a number of bpf information structures that
are then copied from userspace, sometimes from smaller memory locations
than the size of the structure.

Reported-by: Daniel Borkmann <daniel@iogearbox.net
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Note, this is separate from my previous patch, both are needed.

 kernel/bpf/btf.c     | 3 ++-
 kernel/bpf/syscall.c | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 787140095e58..2fc945fcf952 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4564,7 +4564,7 @@ int btf_get_info_by_fd(const struct btf *btf,
 		       union bpf_attr __user *uattr)
 {
 	struct bpf_btf_info __user *uinfo;
-	struct bpf_btf_info info = {};
+	struct bpf_btf_info info;
 	u32 info_copy, btf_copy;
 	void __user *ubtf;
 	u32 uinfo_len;
@@ -4573,6 +4573,7 @@ int btf_get_info_by_fd(const struct btf *btf,
 	uinfo_len = attr->info.info_len;
 
 	info_copy = min_t(u32, uinfo_len, sizeof(info));
+	memset(&info, 0, sizeof(info));
 	if (copy_from_user(&info, uinfo, info_copy))
 		return -EFAULT;
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a4b1de8ea409..84213cc5d016 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2787,7 +2787,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
 				   union bpf_attr __user *uattr)
 {
 	struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
-	struct bpf_prog_info info = {};
+	struct bpf_prog_info info;
 	u32 info_len = attr->info.info_len;
 	struct bpf_prog_stats stats;
 	char __user *uinsns;
@@ -2799,6 +2799,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
 		return err;
 	info_len = min_t(u32, sizeof(info), info_len);
 
+	memset(&info, 0, sizeof(info));
 	if (copy_from_user(&info, uinfo, info_len))
 		return -EFAULT;
 
@@ -3062,7 +3063,7 @@ static int bpf_map_get_info_by_fd(struct bpf_map *map,
 				  union bpf_attr __user *uattr)
 {
 	struct bpf_map_info __user *uinfo = u64_to_user_ptr(attr->info.info);
-	struct bpf_map_info info = {};
+	struct bpf_map_info info;
 	u32 info_len = attr->info.info_len;
 	int err;
 
@@ -3071,6 +3072,7 @@ static int bpf_map_get_info_by_fd(struct bpf_map *map,
 		return err;
 	info_len = min_t(u32, sizeof(info), info_len);
 
+	memset(&info, 0, sizeof(info));
 	info.type = map->map_type;
 	info.id = map->id;
 	info.key_size = map->key_size;
-- 
2.25.2

Re: [PATCH] bpf: explicitly memset some bpf info structures declared on the stack

[Yonghong Song]

On 3/20/20 9:22 AM, Greg Kroah-Hartman wrote:
> Trying to initialize a structure with "= {};" will not always clean out
> all padding locations in a structure.  So be explicit and call memset to
> initialize everything for a number of bpf information structures that
> are then copied from userspace, sometimes from smaller memory locations
> than the size of the structure.
> 
> Reported-by: Daniel Borkmann <daniel@iogearbox.net
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH] bpf: explicitly memset some bpf info structures declared on the stack

[Daniel Borkmann]

On 3/20/20 5:22 PM, Greg Kroah-Hartman wrote:
> Trying to initialize a structure with "= {};" will not always clean out
> all padding locations in a structure.  So be explicit and call memset to
> initialize everything for a number of bpf information structures that
> are then copied from userspace, sometimes from smaller memory locations
> than the size of the structure.
> 
> Reported-by: Daniel Borkmann <daniel@iogearbox.net
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied, thanks!

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

[Daniel Borkmann]

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>

Applied, thanks!