[PATCH] bpf: btf: Fix a missing-check bug

[PATCH] bpf: btf: Fix a missing-check bug

[Wenwen Wang]

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)));
+
 	err = btf_parse_str_sec(env);
 	if (err)
 		goto errout;
-- 
2.7.4

Re: [PATCH] bpf: btf: Fix a missing-check bug

[Martin Lau]

On Fri, Oct 19, 2018 at 05:29:51PM -0500, Wenwen Wang 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.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH] bpf: btf: Fix a missing-check bug

[Y Song]

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?

> +
>         err = btf_parse_str_sec(env);
>         if (err)
>                 goto errout;
> --
> 2.7.4
>

Re: [PATCH] bpf: btf: Fix a missing-check bug

[Daniel Borkmann]

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

Re: [PATCH] bpf: btf: Fix a missing-check bug

[Wenwen Wang]

On Wed, Oct 24, 2018 at 4:39 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> 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.

Hi Yonghong and Daniel,

Thanks for your suggestions! No problem, I will work on the v2 and
resubmit the patch.

Wenwen