[Linux-kernel-mentees] [PATCH] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Linux-kernel-mentees] [PATCH] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Peilin Ye]

Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
as NULL. This patch fixes the following syzbot bug:

    https://syzkaller.appspot.com/bug?id=5edd146856fd513747c1992442732e5a0e9ba355

Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 kernel/bpf/btf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 30721f2c2d10..3e981b183fa4 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4088,7 +4088,7 @@ static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
 	const char *tname, *sym;
 	u32 btf_id, i;
 
-	if (IS_ERR(btf_vmlinux)) {
+	if (IS_ERR_OR_NULL(btf_vmlinux)) {
 		bpf_log(log, "btf_vmlinux is malformed\n");
 		return -EINVAL;
 	}
-- 
2.25.1

Re: [Linux-kernel-mentees] [PATCH] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Andrii Nakryiko]

On Mon, Jul 13, 2020 at 5:43 PM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
> as NULL. This patch fixes the following syzbot bug:
>
>     https://syzkaller.appspot.com/bug?id=5edd146856fd513747c1992442732e5a0e9ba355
>
> Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
>  kernel/bpf/btf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 30721f2c2d10..3e981b183fa4 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4088,7 +4088,7 @@ static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
>         const char *tname, *sym;
>         u32 btf_id, i;
>
> -       if (IS_ERR(btf_vmlinux)) {
> +       if (IS_ERR_OR_NULL(btf_vmlinux)) {
>                 bpf_log(log, "btf_vmlinux is malformed\n");

Can you please split IS_ERR and NULL cases and emit different messages
to log? If the kernel is not built with btf_vmlinux, saying that it's
"malformed" will confuse people. Thanks.

>                 return -EINVAL;
>         }
> --
> 2.25.1
>

[Linux-kernel-mentees] [PATCH v2] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Peilin Ye]

Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
as NULL. This patch fixes the following syzbot bug:

    https://syzkaller.appspot.com/bug?id=5edd146856fd513747c1992442732e5a0e9ba355

Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Thank you for reviewing my patch! I am new to Linux kernel development; would
the log message and errno be appropriate for this case?

Change in v2:
    - Split NULL and IS_ERR cases.

 kernel/bpf/btf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 30721f2c2d10..092116a311f4 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4088,6 +4088,11 @@ static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
 	const char *tname, *sym;
 	u32 btf_id, i;
 
+	if (!btf_vmlinux) {
+		bpf_log(log, "btf_vmlinux doesn't exist\n");
+		return -EINVAL;
+	}
+
 	if (IS_ERR(btf_vmlinux)) {
 		bpf_log(log, "btf_vmlinux is malformed\n");
 		return -EINVAL;
-- 
2.25.1

Re: [Linux-kernel-mentees] [PATCH v2] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Andrii Nakryiko]

On Mon, Jul 13, 2020 at 6:29 PM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
> as NULL. This patch fixes the following syzbot bug:
>
>     https://syzkaller.appspot.com/bug?id=5edd146856fd513747c1992442732e5a0e9ba355
>
> Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Thank you for reviewing my patch! I am new to Linux kernel development; would
> the log message and errno be appropriate for this case?

I think it's good enough, thanks for the fix.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> Change in v2:
>     - Split NULL and IS_ERR cases.
>
>  kernel/bpf/btf.c | 5 +++++
>  1 file changed, 5 insertions(+)
>

[...]

Re: [Linux-kernel-mentees] [PATCH v2] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Alexei Starovoitov]

On Mon, Jul 13, 2020 at 9:38 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 13, 2020 at 6:29 PM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> >
> > Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
> > as NULL. This patch fixes the following syzbot bug:
> >
> >     https://syzkaller.appspot.com/bug?id=5edd146856fd513747c1992442732e5a0e9ba355

The link looks wrong?
Nothing in the stack trace indicates this issue.

> > Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> > Thank you for reviewing my patch! I am new to Linux kernel development; would
> > the log message and errno be appropriate for this case?
>
> I think it's good enough, thanks for the fix.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >
> > Change in v2:
> >     - Split NULL and IS_ERR cases.
> >
> >  kernel/bpf/btf.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
>
> [...]

[Linux-kernel-mentees] [PATCH v3] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Peilin Ye]

Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
as NULL. This patch fixes the following syzbot bug:

    https://syzkaller.appspot.com/bug?id=f823224ada908fa5c207902a5a62065e53ca0fcc

Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Sorry, I got the link wrong. Thank you for pointing that out.

Change in v3:
    - Fix incorrect syzbot dashboard link.

Change in v2:
    - Split NULL and IS_ERR cases.

 kernel/bpf/btf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 30721f2c2d10..092116a311f4 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4088,6 +4088,11 @@ static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
 	const char *tname, *sym;
 	u32 btf_id, i;
 
+	if (!btf_vmlinux) {
+		bpf_log(log, "btf_vmlinux doesn't exist\n");
+		return -EINVAL;
+	}
+
 	if (IS_ERR(btf_vmlinux)) {
 		bpf_log(log, "btf_vmlinux is malformed\n");
 		return -EINVAL;
-- 
2.25.1

Re: [Linux-kernel-mentees] [PATCH v3] bpf: Fix NULL pointer dereference in __btf_resolve_helper_id()

[Daniel Borkmann]

On 7/14/20 8:09 PM, Peilin Ye wrote:
> Prevent __btf_resolve_helper_id() from dereferencing `btf_vmlinux`
> as NULL. This patch fixes the following syzbot bug:
> 
>      https://syzkaller.appspot.com/bug?id=f823224ada908fa5c207902a5a62065e53ca0fcc
> 
> Reported-by: syzbot+ee09bda7017345f1fbe6@syzkaller.appspotmail.com
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

Looks good, applied, thanks! As far as I can tell all the other occurrences are
gated behind btf_parse_vmlinux() where we also init struct_opts, etc.

So for bpf-next this would then end up looking like ...

int btf_resolve_helper_id(struct bpf_verifier_log *log,
                           const struct bpf_func_proto *fn, int arg)
{
         int id;

         if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
                 return -EINVAL;
         id = fn->btf_id[arg];
         if (!id || !btf_vmlinux || id > btf_vmlinux->nr_types)
                 return -EINVAL;
         return id;
}

> ---
> Sorry, I got the link wrong. Thank you for pointing that out.
> 
> Change in v3:
>      - Fix incorrect syzbot dashboard link.
> 
> Change in v2:
>      - Split NULL and IS_ERR cases.
> 
>   kernel/bpf/btf.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 30721f2c2d10..092116a311f4 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4088,6 +4088,11 @@ static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
>   	const char *tname, *sym;
>   	u32 btf_id, i;
>   
> +	if (!btf_vmlinux) {
> +		bpf_log(log, "btf_vmlinux doesn't exist\n");
> +		return -EINVAL;
> +	}
> +
>   	if (IS_ERR(btf_vmlinux)) {
>   		bpf_log(log, "btf_vmlinux is malformed\n");
>   		return -EINVAL;
>