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

On 3/20/20 8:24 AM, Daniel Borkmann wrote:

On 3/20/20 10:48 AM, Greg Kroah-Hartman wrote:

quoted

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 <redacted>
Reported-by: John Stultz <redacted>
Reported-by: Alexander Potapenko <glider@google.com>
Reported-by: Alistair Delva <redacted>
Cc: stable <redacted>
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.

quoted

      if (copy_from_user(&attr, uattr, size) != 0)
          return -EFAULT;

base-commit: 6c90b86a745a446717fdf408c4a8a4631a5e8ee3