bpf: explicitly memset the bpf_attr structure
Subsystems (5)
Greg Kroah-Hartman
maintainerPATCH 1/1 bpf: explicitly memset the bpf_attr structure
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.
--- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/bpf/syscall.c +2 -1
@@ -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
Signature · 1 line
2.25.2
Quoted 5 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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)).
Quoted 10 lines
Fix this by explicitly memsetting the structure to 0 before using it.
Reported-by Maciej Żenczykowski <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 28 lines
--- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/syscall.c +2
-1@@ -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
Greg Kroah-Hartman
maintainerQuoted 11 lines · Yonghong Song
Quoted 5 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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.
Quoted 4 lines
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 beyondmin(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
Daniel Borkmann
maintainerQuoted 33 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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 <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>--- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/syscall.c +1
-1@@ -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 ))
kernel/bpf/syscall.c SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz · line 3369
Quoted 4 lines
kernel/bpf/syscall.c +1
-0@@ -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.
Quoted 6 lines
kernel/bpf/syscall.c +0
-0if (copy_from_user (&attr, uattr, size) !=0 )return -EFAULT ;base-commit:
6c90b86a745a446717fdf408c4a8a4631a5e8ee3
Quoted 40 lines · Daniel Borkmann
Quoted 31 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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 <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>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 <[email protected]>---
kernel/bpf/syscall.c| 3 ++-1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/syscall.c +1
-1@@ -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_attrQuoted 4 lines
__user*, uattr, unsigned int, siz
size =min_t(u32, size, sizeof(attr));
/* copy attributes from user space, may be less thansizeof(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:
Quoted 3 lines
-
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.
Quoted 9 lines
Please add these as well to your fix.
Quoted 5 lines
if (
copy_from_user(&attr, uattr, size)!= 0) return-EFAULT;base-commit:
6c90b86a745a446717fdf408c4a8a4631a5e8ee3
Greg Kroah-Hartman
maintainerQuoted 39 lines · Daniel Borkmann
Quoted 31 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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 <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>--- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/syscall.c +1
-1@@ -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 ))kernel/bpf/syscall.c SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz · line 3368
Quoted 3 lines
kernel/bpf/syscall.c +1
-0@@ -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:
Quoted 1 line
-
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.
Quoted 1 line
-
bpf_map_get_info_by_fd()
No padding in struct bpf_map_info that I can see, so I doubt this is needed there.
Quoted 1 line
-
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
Daniel Borkmann
maintainerQuoted 40 lines · Greg Kroah-Hartman
Quoted 39 lines · Daniel Borkmann
Quoted 31 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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 <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>--- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/syscall.c +1
-1@@ -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 ))kernel/bpf/syscall.c SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz · line 3368
Quoted 3 lines
kernel/bpf/syscall.c +1
-0@@ -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:
Quoted 4 lines
Quoted 1 line
-
bpf_prog_get_info_by_fd()Unless I am mistaken, struct
bpf_prog_infois 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.
Quoted 5 lines
Quoted 1 line
-
bpf_map_get_info_by_fd()No padding in struct
bpf_map_infothat I can see, so I doubt this is needed there.
Quoted 8 lines
Quoted 1 line
-
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 theCHECK_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
Greg Kroah-Hartman
maintainerQuoted 59 lines · Daniel Borkmann
Quoted 40 lines · Greg Kroah-Hartman
Quoted 39 lines · Daniel Borkmann
Quoted 31 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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 <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>--- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/syscall.c +1
-1@@ -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 ))kernel/bpf/syscall.c SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz · line 3368
Quoted 3 lines
kernel/bpf/syscall.c +1
-0@@ -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:
Quoted 4 lines
Quoted 1 line
-
bpf_prog_get_info_by_fd()Unless I am mistaken, struct
bpf_prog_infois 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.
Quoted 10 lines
Quoted 1 line
-
bpf_map_get_info_by_fd()No padding in struct
bpf_map_infothat I can see, so I doubt this is needed there.Quoted 1 line
-
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.
Quoted 8 lines
Quoted 3 lines
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 theCHECK_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: Addgpl_compatibleflag to structbpf_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
Greg Kroah-Hartman
maintainerPATCH 1/1 bpf: explicitly memset some bpf info structures declared on the stack
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.
---
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(-)
kernel/bpf/btf.c +2 -1
@@ -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 ;
kernel/bpf/syscall.c +4 -2
@@ -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;
Signature · 1 line
2.25.2
Re: [PATCH] bpf: explicitly memset some bpf info structures declared on the stack
Quoted 8 lines · 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 <[email protected]Signed-off-by Greg Kroah-Hartman <[email protected]>
Re: [PATCH] bpf: explicitly memset some bpf info structures declared on the stack
In reply to Greg Kroah-HartmanQuoted 8 lines · 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 <[email protected]Signed-off-by Greg Kroah-Hartman <[email protected]>
Applied, thanks!
Sasha Levin
maintainerHi
[This is an automated email]
This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.5.10, v5.4.26, v4.19.111, v4.14.173, v4.9.216, v4.4.216.
v5.5.10: Build OK!
v5.4.26: Build OK!
v4.19.111: Build OK!
v4.14.173: Build OK!
v4.9.216: Failed to apply! Possible dependencies:
Unable to calculate
v4.4.216: Failed to apply! Possible dependencies:
Unable to calculate
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
Signature · 2 lines
Thanks Sasha
Quoted 8 lines · Greg Kroah-Hartman
For the bpf syscall, we are relying on the compiler to properly zero out the
bpf_attrunion 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.
Quoted 7 lines
Reported-by Maciej Żenczykowski <[email protected]>Reported-by John Stultz <[email protected]>Reported-by Alexander Potapenko <[email protected]>Reported-by Alistair Delva <[email protected]>Cc stable <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>
Applied, thanks!