netdev - Re: [PATCH] bpf: devmap: Check attr->max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Oct 2017 20:52:13 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Richard Weinberger <richard@....at>, netdev@...r.kernel.org
CC:     linux-kernel@...r.kernel.org, ast@...nel.org, sp3485@...umbia.edu,
        tj@...nel.org, mark.rutland@....com, john.fastabend@...il.com
Subject: Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully

[ +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@...umbia.edu>
> Reported-by: syzkaller <syzkaller@...glegroups.com>
> Signed-off-by: Richard Weinberger <richard@....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