[PATCH bpf-next] bpf: fix sock_map_alloc() error path

[PATCH bpf-next] bpf: fix sock_map_alloc() error path

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

In case user program provides silly parameters, we want
a map_alloc() handler to return an error, not a NULL pointer,
otherwise we crash later in find_and_alloc_map()

Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 kernel/bpf/sockmap.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 48c33417d13c0ad40154f25aeade0c9b4cafd96a..a927e89dad6e9591066c3a87afc497a196ebd887 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -521,8 +521,8 @@ static struct smap_psock *smap_init_psock(struct sock *sock,
 static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
 {
 	struct bpf_stab *stab;
-	int err = -EINVAL;
 	u64 cost;
+	int err;
 
 	if (!capable(CAP_NET_ADMIN))
 		return ERR_PTR(-EPERM);
@@ -547,6 +547,7 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
 
 	/* make sure page count doesn't overflow */
 	cost = (u64) stab->map.max_entries * sizeof(struct sock *);
+	err = -EINVAL;
 	if (cost >= U32_MAX - PAGE_SIZE)
 		goto free_stab;
 

Re: [PATCH bpf-next] bpf: fix sock_map_alloc() error path

[Eric Dumazet]

On Tue, 2018-02-13 at 15:33 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> In case user program provides silly parameters, we want
> a map_alloc() handler to return an error, not a NULL pointer,
> otherwise we crash later in find_and_alloc_map()
> 
> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> ---

This would apply to net or bpf trees, not -next ones, sorry for the
confusion in the [PATCH bpf-next] prefix.

Bug was added in 4.14

Re: [PATCH bpf-next] bpf: fix sock_map_alloc() error path

[Daniel Borkmann]

Hi Eric,

On 02/14/2018 12:43 AM, Eric Dumazet wrote:
> On Tue, 2018-02-13 at 15:33 -0800, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> In case user program provides silly parameters, we want
>> a map_alloc() handler to return an error, not a NULL pointer,
>> otherwise we crash later in find_and_alloc_map()
>>
>> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Reported-by: syzbot <syzkaller@googlegroups.com>
>> ---
> 
> This would apply to net or bpf trees, not -next ones, sorry for the
> confusion in the [PATCH bpf-next] prefix.

No problem, thanks for the fix!

> Bug was added in 4.14

Fixes tag is actually slightly different, it would be:

Fixes: 1aa12bdf1bfb ("bpf: sockmap, add sock close() hook to remove socks")

I can change it if you want, no need to resend.

Thanks,
Daniel

Re: [PATCH bpf-next] bpf: fix sock_map_alloc() error path

[John Fastabend]

On 02/13/2018 05:19 PM, Daniel Borkmann wrote:
> Hi Eric,
> 
> On 02/14/2018 12:43 AM, Eric Dumazet wrote:
>> On Tue, 2018-02-13 at 15:33 -0800, Eric Dumazet wrote:
>>> From: Eric Dumazet <edumazet@google.com>
>>>
>>> In case user program provides silly parameters, we want
>>> a map_alloc() handler to return an error, not a NULL pointer,
>>> otherwise we crash later in find_and_alloc_map()
>>>
>>> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>> Reported-by: syzbot <syzkaller@googlegroups.com>
>>> ---

Thanks Eric, also great to see syzkaller working on the sockmap
types.

Acked-by: John Fastabend <john.fastabend@gmail.com>

>>
>> This would apply to net or bpf trees, not -next ones, sorry for the
>> confusion in the [PATCH bpf-next] prefix.
> 
> No problem, thanks for the fix!
> 
>> Bug was added in 4.14
> 
> Fixes tag is actually slightly different, it would be:
> 
> Fixes: 1aa12bdf1bfb ("bpf: sockmap, add sock close() hook to remove socks")
> 

Yep.

> I can change it if you want, no need to resend.> Thanks,
> Daniel
> 

Re: [PATCH bpf-next] bpf: fix sock_map_alloc() error path

[Alexei Starovoitov]

On Tue, Feb 13, 2018 at 06:53:36PM -0800, John Fastabend wrote:
> On 02/13/2018 05:19 PM, Daniel Borkmann wrote:
> > Hi Eric,
> > 
> > On 02/14/2018 12:43 AM, Eric Dumazet wrote:
> >> On Tue, 2018-02-13 at 15:33 -0800, Eric Dumazet wrote:
> >>> From: Eric Dumazet <edumazet@google.com>
> >>>
> >>> In case user program provides silly parameters, we want
> >>> a map_alloc() handler to return an error, not a NULL pointer,
> >>> otherwise we crash later in find_and_alloc_map()
> >>>
> >>> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
> >>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> >>> Reported-by: syzbot <syzkaller@googlegroups.com>
> >>> ---
> 
> Thanks Eric, also great to see syzkaller working on the sockmap
> types.
> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> 
> >>
> >> This would apply to net or bpf trees, not -next ones, sorry for the
> >> confusion in the [PATCH bpf-next] prefix.
> > 
> > No problem, thanks for the fix!
> > 
> >> Bug was added in 4.14
> > 
> > Fixes tag is actually slightly different, it would be:
> > 
> > Fixes: 1aa12bdf1bfb ("bpf: sockmap, add sock close() hook to remove socks")
> > 
> 
> Yep.

Applied to bpf tree with corrected Fixes tag, Thanks everyone.