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 copies the parsed header from
'btf->hdr' to 'data'. The remaining part in 'data' is still copied from the
user-space 'btf_data'.
Signed-off-by: Wenwen Wang <[email protected]>
---
kernel/bpf/btf.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 378cef7..b52a834a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2152,6 +2152,7 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
struct btf_verifier_env *env = NULL;
struct bpf_verifier_log *log;
struct btf *btf = NULL;
+ u32 hdr_len;
u8 *data;
int err;
@@ -2200,7 +2201,15 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
btf->data_size = btf_data_size;
btf->nohdr_data = btf->data + btf->hdr.hdr_len;
- if (copy_from_user(data, btf_data, btf_data_size)) {
+ /*
+ * The header at btf_data could be modified by a malicious user
+ * after it is parsed. So we copy the parsed header here. The
+ * remaining part is still copied from btf_data.
+ */
+ hdr_len = min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr));
+ memcpy(data, &btf->hdr, hdr_len);
+ if (copy_from_user(data + hdr_len, (u8 __user *)btf_data + hdr_len,
+ btf_data_size - hdr_len)) {
err = -EFAULT;
goto errout;
}
--
2.7.4
On Wed, Oct 24, 2018 at 08:00:19AM -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 copies the parsed header from
> 'btf->hdr' to 'data'. The remaining part in 'data' is still copied from the
> user-space 'btf_data'.
LGTM.
Acked-by: Martin KaFai Lau <[email protected]>
>
> Signed-off-by: Wenwen Wang <[email protected]>
> ---
> kernel/bpf/btf.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 378cef7..b52a834a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -2152,6 +2152,7 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> struct btf_verifier_env *env = NULL;
> struct bpf_verifier_log *log;
> struct btf *btf = NULL;
> + u32 hdr_len;
> u8 *data;
> int err;
>
> @@ -2200,7 +2201,15 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> btf->data_size = btf_data_size;
> btf->nohdr_data = btf->data + btf->hdr.hdr_len;
>
> - if (copy_from_user(data, btf_data, btf_data_size)) {
> + /*
> + * The header at btf_data could be modified by a malicious user
> + * after it is parsed. So we copy the parsed header here. The
> + * remaining part is still copied from btf_data.
> + */
> + hdr_len = min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr));
> + memcpy(data, &btf->hdr, hdr_len);
> + if (copy_from_user(data + hdr_len, (u8 __user *)btf_data + hdr_len,
> + btf_data_size - hdr_len)) {
> err = -EFAULT;
> goto errout;
> }
> --
> 2.7.4
>
On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
> On Wed, Oct 24, 2018 at 08:00:19AM -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.
btw, I am working on a patch that copies the btf_data before parsing/verifying
the header. That should avoid this from happening but that will
require a bit more code churns for the bpf branch.
> >
> > To avoid the above issue, this patch copies the parsed header from
> > 'btf->hdr' to 'data'. The remaining part in 'data' is still copied from the
> > user-space 'btf_data'.
> LGTM.
>
> Acked-by: Martin KaFai Lau <[email protected]>
>
> >
> > Signed-off-by: Wenwen Wang <[email protected]>
> > ---
> > kernel/bpf/btf.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 378cef7..b52a834a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -2152,6 +2152,7 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> > struct btf_verifier_env *env = NULL;
> > struct bpf_verifier_log *log;
> > struct btf *btf = NULL;
> > + u32 hdr_len;
> > u8 *data;
> > int err;
> >
> > @@ -2200,7 +2201,15 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> > btf->data_size = btf_data_size;
> > btf->nohdr_data = btf->data + btf->hdr.hdr_len;
> >
> > - if (copy_from_user(data, btf_data, btf_data_size)) {
> > + /*
> > + * The header at btf_data could be modified by a malicious user
> > + * after it is parsed. So we copy the parsed header here. The
> > + * remaining part is still copied from btf_data.
> > + */
> > + hdr_len = min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr));
> > + memcpy(data, &btf->hdr, hdr_len);
> > + if (copy_from_user(data + hdr_len, (u8 __user *)btf_data + hdr_len,
> > + btf_data_size - hdr_len)) {
> > err = -EFAULT;
> > goto errout;
> > }
> > --
> > 2.7.4
> >
On Wed, Oct 24, 2018 at 06:22:46PM +0000, Martin Lau wrote:
> On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
> > On Wed, Oct 24, 2018 at 08:00:19AM -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.
> btw, I am working on a patch that copies the btf_data before parsing/verifying
> the header. That should avoid this from happening but that will
> require a bit more code churns for the bpf branch.
>
It is what I have in mind:
It is not a good idea to check the BTF header before copying the
user btf_data. The verified header may not be the one actually
copied to btf->data (e.g. userspace may modify the passed in
btf_data in between). Like the one fixed in
commit 8af03d1ae2e1 ("bpf: btf: Fix a missing check bug").
This patch copies the user btf_data before parsing/verifying
the BTF header.
Fixes: 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")
Signed-off-by: Martin KaFai Lau <[email protected]>
---
kernel/bpf/btf.c | 58 +++++++++++++++++++++---------------------------
1 file changed, 25 insertions(+), 33 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 378cef70341c..ee4c82667d65 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2067,56 +2067,47 @@ static int btf_check_sec_info(struct btf_verifier_env *env,
return 0;
}
-static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
- u32 btf_data_size)
+static int btf_parse_hdr(struct btf_verifier_env *env)
{
+ u32 hdr_len, hdr_copy, btf_data_size;
const struct btf_header *hdr;
- u32 hdr_len, hdr_copy;
- /*
- * Minimal part of the "struct btf_header" that
- * contains the hdr_len.
- */
- struct btf_min_header {
- u16 magic;
- u8 version;
- u8 flags;
- u32 hdr_len;
- } __user *min_hdr;
struct btf *btf;
int err;
btf = env->btf;
- min_hdr = btf_data;
+ btf_data_size = btf->data_size;
- if (btf_data_size < sizeof(*min_hdr)) {
+ if (btf_data_size <
+ offsetof(struct btf_header, hdr_len) + sizeof(hdr->hdr_len)) {
btf_verifier_log(env, "hdr_len not found");
return -EINVAL;
}
- if (get_user(hdr_len, &min_hdr->hdr_len))
- return -EFAULT;
-
+ hdr = btf->data;
+ hdr_len = hdr->hdr_len;
if (btf_data_size < hdr_len) {
btf_verifier_log(env, "btf_header not found");
return -EINVAL;
}
- err = bpf_check_uarg_tail_zero(btf_data, sizeof(btf->hdr), hdr_len);
- if (err) {
- if (err == -E2BIG)
- btf_verifier_log(env, "Unsupported btf_header");
- return err;
+ /* Ensure the unsupported header fields are zero */
+ if (hdr_len > sizeof(btf->hdr)) {
+ u8 *expected_zero = btf->data + sizeof(btf->hdr);
+ u8 *end = btf->data + hdr_len;
+
+ for (; expected_zero < end; expected_zero++) {
+ if (*expected_zero) {
+ btf_verifier_log(env, "Unsupported btf_header");
+ return -E2BIG;
+ }
+ }
}
hdr_copy = min_t(u32, hdr_len, sizeof(btf->hdr));
- if (copy_from_user(&btf->hdr, btf_data, hdr_copy))
- return -EFAULT;
+ memcpy(&btf->hdr, btf->data, hdr_copy);
hdr = &btf->hdr;
- if (hdr->hdr_len != hdr_len)
- return -EINVAL;
-
btf_verifier_log_hdr(env, btf_data_size);
if (hdr->magic != BTF_MAGIC) {
@@ -2186,10 +2177,6 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
}
env->btf = btf;
- err = btf_parse_hdr(env, btf_data, btf_data_size);
- if (err)
- goto errout;
-
data = kvmalloc(btf_data_size, GFP_KERNEL | __GFP_NOWARN);
if (!data) {
err = -ENOMEM;
@@ -2198,13 +2185,18 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
btf->data = data;
btf->data_size = btf_data_size;
- btf->nohdr_data = btf->data + btf->hdr.hdr_len;
if (copy_from_user(data, btf_data, btf_data_size)) {
err = -EFAULT;
goto errout;
}
+ err = btf_parse_hdr(env);
+ if (err)
+ goto errout;
+
+ btf->nohdr_data = btf->data + btf->hdr.hdr_len;
+
err = btf_parse_str_sec(env);
if (err)
goto errout;
--
2.17.1
On Wed, Oct 24, 2018 at 1:45 PM Martin Lau <[email protected]> wrote:
>
> On Wed, Oct 24, 2018 at 06:22:46PM +0000, Martin Lau wrote:
> > On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
> > > On Wed, Oct 24, 2018 at 08:00:19AM -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.
> > btw, I am working on a patch that copies the btf_data before parsing/verifying
> > the header. That should avoid this from happening but that will
> > require a bit more code churns for the bpf branch.
> >
> It is what I have in mind:
>
>
> It is not a good idea to check the BTF header before copying the
> user btf_data. The verified header may not be the one actually
> copied to btf->data (e.g. userspace may modify the passed in
> btf_data in between). Like the one fixed in
> commit 8af03d1ae2e1 ("bpf: btf: Fix a missing check bug").
>
> This patch copies the user btf_data before parsing/verifying
> the BTF header.
>
> Fixes: 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")
> Signed-off-by: Martin KaFai Lau <[email protected]>
Acked-by: Song Liu <[email protected]>
> ---
> kernel/bpf/btf.c | 58 +++++++++++++++++++++---------------------------
> 1 file changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 378cef70341c..ee4c82667d65 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -2067,56 +2067,47 @@ static int btf_check_sec_info(struct btf_verifier_env *env,
> return 0;
> }
>
> -static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
> - u32 btf_data_size)
> +static int btf_parse_hdr(struct btf_verifier_env *env)
> {
> + u32 hdr_len, hdr_copy, btf_data_size;
> const struct btf_header *hdr;
> - u32 hdr_len, hdr_copy;
> - /*
> - * Minimal part of the "struct btf_header" that
> - * contains the hdr_len.
> - */
> - struct btf_min_header {
> - u16 magic;
> - u8 version;
> - u8 flags;
> - u32 hdr_len;
> - } __user *min_hdr;
> struct btf *btf;
> int err;
>
> btf = env->btf;
> - min_hdr = btf_data;
> + btf_data_size = btf->data_size;
>
> - if (btf_data_size < sizeof(*min_hdr)) {
> + if (btf_data_size <
> + offsetof(struct btf_header, hdr_len) + sizeof(hdr->hdr_len)) {
> btf_verifier_log(env, "hdr_len not found");
> return -EINVAL;
> }
>
> - if (get_user(hdr_len, &min_hdr->hdr_len))
> - return -EFAULT;
> -
> + hdr = btf->data;
> + hdr_len = hdr->hdr_len;
> if (btf_data_size < hdr_len) {
> btf_verifier_log(env, "btf_header not found");
> return -EINVAL;
> }
>
> - err = bpf_check_uarg_tail_zero(btf_data, sizeof(btf->hdr), hdr_len);
> - if (err) {
> - if (err == -E2BIG)
> - btf_verifier_log(env, "Unsupported btf_header");
> - return err;
> + /* Ensure the unsupported header fields are zero */
> + if (hdr_len > sizeof(btf->hdr)) {
> + u8 *expected_zero = btf->data + sizeof(btf->hdr);
> + u8 *end = btf->data + hdr_len;
> +
> + for (; expected_zero < end; expected_zero++) {
> + if (*expected_zero) {
> + btf_verifier_log(env, "Unsupported btf_header");
> + return -E2BIG;
> + }
> + }
> }
>
> hdr_copy = min_t(u32, hdr_len, sizeof(btf->hdr));
> - if (copy_from_user(&btf->hdr, btf_data, hdr_copy))
> - return -EFAULT;
> + memcpy(&btf->hdr, btf->data, hdr_copy);
>
> hdr = &btf->hdr;
>
> - if (hdr->hdr_len != hdr_len)
> - return -EINVAL;
> -
> btf_verifier_log_hdr(env, btf_data_size);
>
> if (hdr->magic != BTF_MAGIC) {
> @@ -2186,10 +2177,6 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> }
> env->btf = btf;
>
> - err = btf_parse_hdr(env, btf_data, btf_data_size);
> - if (err)
> - goto errout;
> -
> data = kvmalloc(btf_data_size, GFP_KERNEL | __GFP_NOWARN);
> if (!data) {
> err = -ENOMEM;
> @@ -2198,13 +2185,18 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
>
> btf->data = data;
> btf->data_size = btf_data_size;
> - btf->nohdr_data = btf->data + btf->hdr.hdr_len;
>
> if (copy_from_user(data, btf_data, btf_data_size)) {
> err = -EFAULT;
> goto errout;
> }
>
> + err = btf_parse_hdr(env);
> + if (err)
> + goto errout;
> +
> + btf->nohdr_data = btf->data + btf->hdr.hdr_len;
> +
> err = btf_parse_str_sec(env);
> if (err)
> goto errout;
> --
> 2.17.1
>
>
On 10/24/2018 10:42 PM, Martin Lau wrote:
> On Wed, Oct 24, 2018 at 06:22:46PM +0000, Martin Lau wrote:
>> On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
>>> On Wed, Oct 24, 2018 at 08:00:19AM -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.
>> btw, I am working on a patch that copies the btf_data before parsing/verifying
>> the header. That should avoid this from happening but that will
>> require a bit more code churns for the bpf branch.
>>
> It is what I have in mind:
>
> It is not a good idea to check the BTF header before copying the
> user btf_data. The verified header may not be the one actually
> copied to btf->data (e.g. userspace may modify the passed in
> btf_data in between). Like the one fixed in
> commit 8af03d1ae2e1 ("bpf: btf: Fix a missing check bug").
>
> This patch copies the user btf_data before parsing/verifying
> the BTF header.
>
> Fixes: 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")
> Signed-off-by: Martin KaFai Lau <[email protected]>
I've added Co-developed-by tag and applied it to bpf tree, thanks everyone!