| Message ID | 20171015220020.8157-1-richard@nod.at |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | bpf: devmap: Check attr->max_entries more carefully | expand |
Am Montag, 16. Oktober 2017, 00:00:20 CEST schrieb Richard Weinberger: > max_entries is user controlled and used as input for __alloc_percpu(). > This function expects that the allocation size is a power of two and > less than PCPU_MIN_UNIT_SIZE. > Otherwise a WARN() is triggered. On a second though, I think we should also have a hard limit for ->max_entries or check for an overflow here: static u64 dev_map_bitmap_size(const union bpf_attr *attr) { return BITS_TO_LONGS(attr->max_entries) * sizeof(unsigned long); } Thanks, //richard
[ +Tejun, Mark, John ] On 10/16/2017 12:00 AM, Richard Weinberger wrote: > max_entries is user controlled and used as input for __alloc_percpu(). > This function expects that the allocation size is a power of two and > less than PCPU_MIN_UNIT_SIZE. > Otherwise a WARN() is triggered. > > Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map") > Reported-by: Shankara Pailoor <sp3485@columbia.edu> > Reported-by: syzkaller <syzkaller@googlegroups.com> > Signed-off-by: Richard Weinberger <richard@nod.at> Thanks for the patch, Richard. There was a prior discussion here [1] on the same issue, I thought this would have been resolved by now, but looks like it's still open and there was never a follow-up, at least I don't see it in the percpu tree if I didn't miss anything. I would suggest, we do the following below and pass __GFP_NOWARN from BPF side to the per-cpu allocs. This is kind of a generic 'issue' and we shouldn't add more code which bails out anyway just to work around the WARN(). Lets handle it properly instead. If Tejun is fine with the one below, I could cook and official patch and cleanup the remaining call-sites from BPF which have similar pattern. [1] https://patchwork.kernel.org/patch/9975851/ Thanks, Daniel mm/percpu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mm/percpu.c b/mm/percpu.c index 59d44d6..5d9414e 100644 --- a/mm/percpu.c +++ b/mm/percpu.c @@ -1357,7 +1357,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, if (unlikely(!size || size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE || !is_power_of_2(align))) { - WARN(true, "illegal size (%zu) or align (%zu) for percpu allocation\n", + WARN(!(gfp & __GFP_NOWARN), + "illegal size (%zu) or align (%zu) for percpu allocation\n", size, align); return NULL; } @@ -1478,7 +1479,7 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, fail: trace_percpu_alloc_percpu_fail(reserved, is_atomic, size, align); - if (!is_atomic && warn_limit) { + if (!is_atomic && warn_limit && !(gfp & __GFP_NOWARN)) { pr_warn("allocation failed, size=%zu align=%zu atomic=%d, %s\n", size, align, is_atomic, err); dump_stack();
On 10/15/2017 03:13 PM, Richard Weinberger wrote: > Am Montag, 16. Oktober 2017, 00:00:20 CEST schrieb Richard Weinberger: >> max_entries is user controlled and used as input for __alloc_percpu(). >> This function expects that the allocation size is a power of two and >> less than PCPU_MIN_UNIT_SIZE. >> Otherwise a WARN() is triggered. > > On a second though, I think we should also have a hard limit for ->max_entries > or check for an overflow here: > Or just, static u64 dev_map_bitmap_size(const union bpf_attr *attr) { return BITS_TO_LONGS((u64) attr->max_entries) * sizeof(unsigned long); } Let me know if you want me to send a patch or if you want to. Thanks, John > static u64 dev_map_bitmap_size(const union bpf_attr *attr) > { > return BITS_TO_LONGS(attr->max_entries) * sizeof(unsigned long); > } > > Thanks, > //richard >
On Mon, Oct 16, 2017 at 08:52:13PM +0200, Daniel Borkmann wrote: > [ +Tejun, Mark, John ] > > On 10/16/2017 12:00 AM, Richard Weinberger wrote: > > max_entries is user controlled and used as input for __alloc_percpu(). > > This function expects that the allocation size is a power of two and > > less than PCPU_MIN_UNIT_SIZE. > > Otherwise a WARN() is triggered. > > > > Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map") > > Reported-by: Shankara Pailoor <sp3485@columbia.edu> > > Reported-by: syzkaller <syzkaller@googlegroups.com> > > Signed-off-by: Richard Weinberger <richard@nod.at> > > Thanks for the patch, Richard. There was a prior discussion here [1] on > the same issue, I thought this would have been resolved by now, but looks > like it's still open and there was never a follow-up, at least I don't see > it in the percpu tree if I didn't miss anything. Sorry, this was on my todo list, but I've been bogged down with some other work. > I would suggest, we do the following below and pass __GFP_NOWARN from BPF > side to the per-cpu allocs. This is kind of a generic 'issue' and we shouldn't > add more code which bails out anyway just to work around the WARN(). Lets > handle it properly instead. Agreed. The below patch looks good to me, (with the suggested change to the BPF side). > If Tejun is fine with the one below, I could cook and official patch and > cleanup the remaining call-sites from BPF which have similar pattern. That would be great; thanks for taking this on. Thanks, Mark. > > [1] https://patchwork.kernel.org/patch/9975851/ > > Thanks, > Daniel > > mm/percpu.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/percpu.c b/mm/percpu.c > index 59d44d6..5d9414e 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -1357,7 +1357,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > > if (unlikely(!size || size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE || > !is_power_of_2(align))) { > - WARN(true, "illegal size (%zu) or align (%zu) for percpu allocation\n", > + WARN(!(gfp & __GFP_NOWARN), > + "illegal size (%zu) or align (%zu) for percpu allocation\n", > size, align); > return NULL; > } > @@ -1478,7 +1479,7 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > fail: > trace_percpu_alloc_percpu_fail(reserved, is_atomic, size, align); > > - if (!is_atomic && warn_limit) { > + if (!is_atomic && warn_limit && !(gfp & __GFP_NOWARN)) { > pr_warn("allocation failed, size=%zu align=%zu atomic=%d, %s\n", > size, align, is_atomic, err); > dump_stack(); > -- > 1.9.3
On 10/17/2017 12:29 PM, Mark Rutland wrote: > On Mon, Oct 16, 2017 at 08:52:13PM +0200, Daniel Borkmann wrote: >> [ +Tejun, Mark, John ] >> >> On 10/16/2017 12:00 AM, Richard Weinberger wrote: >>> max_entries is user controlled and used as input for __alloc_percpu(). >>> This function expects that the allocation size is a power of two and >>> less than PCPU_MIN_UNIT_SIZE. >>> Otherwise a WARN() is triggered. >>> >>> Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map") >>> Reported-by: Shankara Pailoor <sp3485@columbia.edu> >>> Reported-by: syzkaller <syzkaller@googlegroups.com> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >> >> Thanks for the patch, Richard. There was a prior discussion here [1] on >> the same issue, I thought this would have been resolved by now, but looks >> like it's still open and there was never a follow-up, at least I don't see >> it in the percpu tree if I didn't miss anything. > > Sorry, this was on my todo list, but I've been bogged down with some > other work. Ok, no problem. >> I would suggest, we do the following below and pass __GFP_NOWARN from BPF >> side to the per-cpu allocs. This is kind of a generic 'issue' and we shouldn't >> add more code which bails out anyway just to work around the WARN(). Lets >> handle it properly instead. > > Agreed. The below patch looks good to me, (with the suggested change to > the BPF side). > >> If Tejun is fine with the one below, I could cook and official patch and >> cleanup the remaining call-sites from BPF which have similar pattern. > > That would be great; thanks for taking this on. I'll prepare a set including BPF side for today. Thanks, Daniel
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index e093d9a2c4dd..6ce00083103b 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -49,6 +49,7 @@ */ #include <linux/bpf.h> #include <linux/filter.h> +#include <linux/log2.h> struct bpf_dtab_netdev { struct net_device *dev; @@ -77,6 +78,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr) struct bpf_dtab *dtab; int err = -EINVAL; u64 cost; + size_t palloc_size; /* check sanity of attributes */ if (attr->max_entries == 0 || attr->key_size != 4 || @@ -95,9 +97,14 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr) dtab->map.map_flags = attr->map_flags; dtab->map.numa_node = bpf_map_attr_numa_node(attr); + palloc_size = roundup_pow_of_two(dev_map_bitmap_size(attr)); + if (palloc_size > PCPU_MIN_UNIT_SIZE || + palloc_size < dev_map_bitmap_size(attr)) + return ERR_PTR(-EINVAL); + /* make sure page count doesn't overflow */ cost = (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *); - cost += dev_map_bitmap_size(attr) * num_possible_cpus(); + cost += palloc_size * num_possible_cpus(); if (cost >= U32_MAX - PAGE_SIZE) goto free_dtab; @@ -111,7 +118,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr) err = -ENOMEM; /* A per cpu bitfield with a bit per possible net device */ - dtab->flush_needed = __alloc_percpu(dev_map_bitmap_size(attr), + dtab->flush_needed = __alloc_percpu(palloc_size, __alignof__(unsigned long)); if (!dtab->flush_needed) goto free_dtab;
max_entries is user controlled and used as input for __alloc_percpu(). This function expects that the allocation size is a power of two and less than PCPU_MIN_UNIT_SIZE. Otherwise a WARN() is triggered. Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map") Reported-by: Shankara Pailoor <sp3485@columbia.edu> Reported-by: syzkaller <syzkaller@googlegroups.com> Signed-off-by: Richard Weinberger <richard@nod.at> --- kernel/bpf/devmap.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)