Messages in this thread | | | Subject | Re: [PATCH] bpf: btf: Fix a missing-check bug | From | Daniel Borkmann <> | Date | Wed, 24 Oct 2018 11:16:38 +0200 |
| |
Hi Wenwen,
On 10/22/2018 05:57 PM, Y Song wrote: > On Fri, Oct 19, 2018 at 3:30 PM Wenwen Wang <wang6495@umn.edu> wrote: >> >> In btf_parse(), the header of the user-space btf data 'btf_data' is firstly >> parsed and verified through btf_parse_hdr(). In btf_parse_hdr(), the header >> is copied from user-space 'btf_data' to kernel-space 'btf->hdr' and then >> verified. If no error happens during the verification process, the whole >> data of 'btf_data', including the header, is then copied to 'data' in >> btf_parse(). It is obvious that the header is copied twice here. More >> importantly, no check is enforced after the second copy to make sure the >> headers obtained in these two copies are same. Given that 'btf_data' >> resides in the user space, a malicious user can race to modify the header >> between these two copies. By doing so, the user can inject inconsistent >> data, which can cause undefined behavior of the kernel and introduce >> potential security risk. >> >> To avoid the above issue, this patch rewrites the header after the second >> copy, using 'btf->hdr', which is obtained in the first copy. >> >> Signed-off-by: Wenwen Wang <wang6495@umn.edu> >> --- >> kernel/bpf/btf.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >> index 138f030..2a85f91 100644 >> --- a/kernel/bpf/btf.c >> +++ b/kernel/bpf/btf.c >> @@ -2202,6 +2202,9 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size, >> goto errout; >> } >> >> + memcpy(data, &btf->hdr, >> + min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr))); > > Could you restructure the code to memcpy the header followed by copying > the rest of btf_data with copy_from_user? This way, each byte is only > copied once. > Could you add some comments right before memcpy so later people will know > why we implement this way?
Thanks for the fix! Agree with Yonghong that we should rework this a bit, so please respin a v2 with the feedback addressed, thanks.
>> + >> err = btf_parse_str_sec(env); >> if (err) >> goto errout; >> -- >> 2.7.4 >>
| |