[PATCH] bpf: devmap: Check attr->max_entries more carefully

[PATCH] bpf: devmap: Check attr->max_entries more carefully

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

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

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

Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully

[Richard Weinberger]

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

Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully

[Daniel Borkmann]

[ +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();
-- 
1.9.3

Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully

[John Fastabend]

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
> 

Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully

[Mark Rutland]

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

Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully

[Daniel Borkmann]

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