| Message ID | 20200821191054.714731-1-yhs@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: fix a buffer out-of-bound access when filling raw_tp link_info | expand |
On Fri, Aug 21, 2020 at 12:11 PM Yonghong Song <yhs@fb.com> wrote: > > Commit f2e10bff16a0 ("bpf: Add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link") > added link query for raw_tp. One of fields in link_info is to > fill a user buffer with tp_name. The Scurrent checking only > declares "ulen && !ubuf" as invalid. So "!ulen && ubuf" will be > valid. Later on, we do "copy_to_user(ubuf, tp_name, ulen - 1)" which > may overwrite user memory incorrectly. > > This patch fixed the problem by disallowing "!ulen && ubuf" case as well. > > Fixes: f2e10bff16a0 ("bpf: Add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link") > Cc: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > kernel/bpf/syscall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 86299a292214..ac6c784c0576 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2634,7 +2634,7 @@ static int bpf_raw_tp_link_fill_link_info(const struct bpf_link *link, > u32 ulen = info->raw_tracepoint.tp_name_len; > size_t tp_len = strlen(tp_name); > > - if (ulen && !ubuf) > + if (!ulen ^ !ubuf) > return -EINVAL; I think my original idea was to allow ulen == 0 && ubuf != NULL as a still valid way to get real ulen, but it's clearly wrong with ulen-1 below. So instead of special-casing ulen==0 for the case I wanted to support, it's easier to disallow ulen==0 && ubuf!=NULL. So thanks for the fix! Acked-by: Andrii Nakryiko <andriin@fb.com> > > info->raw_tracepoint.tp_name_len = tp_len + 1; > -- > 2.24.1 >
On Fri, Aug 21, 2020 at 1:09 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Aug 21, 2020 at 12:11 PM Yonghong Song <yhs@fb.com> wrote: > > > > Commit f2e10bff16a0 ("bpf: Add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link") > > added link query for raw_tp. One of fields in link_info is to > > fill a user buffer with tp_name. The Scurrent checking only > > declares "ulen && !ubuf" as invalid. So "!ulen && ubuf" will be > > valid. Later on, we do "copy_to_user(ubuf, tp_name, ulen - 1)" which > > may overwrite user memory incorrectly. > > > > This patch fixed the problem by disallowing "!ulen && ubuf" case as well. > > > > Fixes: f2e10bff16a0 ("bpf: Add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link") > > Cc: Andrii Nakryiko <andriin@fb.com> > > Signed-off-by: Yonghong Song <yhs@fb.com> > > --- > > kernel/bpf/syscall.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 86299a292214..ac6c784c0576 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -2634,7 +2634,7 @@ static int bpf_raw_tp_link_fill_link_info(const struct bpf_link *link, > > u32 ulen = info->raw_tracepoint.tp_name_len; > > size_t tp_len = strlen(tp_name); > > > > - if (ulen && !ubuf) > > + if (!ulen ^ !ubuf) > > return -EINVAL; > > I think my original idea was to allow ulen == 0 && ubuf != NULL as a > still valid way to get real ulen, but it's clearly wrong with ulen-1 > below. So instead of special-casing ulen==0 for the case I wanted to > support, it's easier to disallow ulen==0 && ubuf!=NULL. > > So thanks for the fix! > > Acked-by: Andrii Nakryiko <andriin@fb.com> Applied. Thanks
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 86299a292214..ac6c784c0576 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2634,7 +2634,7 @@ static int bpf_raw_tp_link_fill_link_info(const struct bpf_link *link, u32 ulen = info->raw_tracepoint.tp_name_len; size_t tp_len = strlen(tp_name); - if (ulen && !ubuf) + if (!ulen ^ !ubuf) return -EINVAL; info->raw_tracepoint.tp_name_len = tp_len + 1;
Commit f2e10bff16a0 ("bpf: Add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link") added link query for raw_tp. One of fields in link_info is to fill a user buffer with tp_name. The Scurrent checking only declares "ulen && !ubuf" as invalid. So "!ulen && ubuf" will be valid. Later on, we do "copy_to_user(ubuf, tp_name, ulen - 1)" which may overwrite user memory incorrectly. This patch fixed the problem by disallowing "!ulen && ubuf" case as well. Fixes: f2e10bff16a0 ("bpf: Add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link") Cc: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)