[PATCH] netlink: fix null pointer dereference on nlk->groups

[PATCH] netlink: fix null pointer dereference on nlk->groups

[Baozeng Ding]

If groups is not 0 and nlk->groups is NULL, it will not return
immediately and cause a null pointer dereference later.

Signed-off-by: Baozeng Ding <sploving1@gmail.com>
---
 net/netlink/af_netlink.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 59651af..38efde0 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1524,6 +1524,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	int err;
 	long unsigned int groups = nladdr->nl_groups;
 	bool bound;
+	unsigned long nlgroups;
 
 	if (addr_len < sizeof(struct sockaddr_nl))
 		return -EINVAL;
@@ -1576,14 +1577,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 		}
 	}
 
-	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
+	if (nlk->groups == NULL)
+		return 0;
+	nlgroups = nlk->groups[0];
+	if (!groups && !(u32)nlgroups)
 		return 0;
 
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(groups) -
-					 hweight32(nlk->groups[0]));
-	nlk->groups[0] = (nlk->groups[0] & ~0xffffffffUL) | groups;
+					 hweight32(nlgroups));
+	nlk->groups[0] = (nlgroups & ~0xffffffffUL) | groups;
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-- 
1.9.1

Re: [PATCH] netlink: fix null pointer dereference on nlk->groups

[Sergei Shtylyov]

Hello.

On 01/08/2016 08:46 AM, Baozeng Ding wrote:

> If groups is not 0 and nlk->groups is NULL, it will not return
> immediately and cause a null pointer dereference later.
>
> Signed-off-by: Baozeng Ding <sploving1@gmail.com>
> ---
>   net/netlink/af_netlink.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 59651af..38efde0 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
[...]
> @@ -1576,14 +1577,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
>   		}
>   	}
>
> -	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> +	if (nlk->groups == NULL)

    '!nlk->groups' is preferred in the networking code.

[...]

MBR, Sergei

[PATCH v2] netlink: fix null pointer dereference on nlk->groups

[Baozeng Ding]

If groups is not 0 and nlk->groups is NULL, it will not return
immediately and cause a null pointer dereference later.

Signed-off-by: Baozeng Ding <sploving1@gmail.com>
---
This version uses the preferred networking coding style. Thanks
for Sergei's feedback. Also the patch keeps the original author's
coding style as much as possible.
---
 net/netlink/af_netlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 59651af..eeff14a 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1576,7 +1576,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 		}
 	}
 
-	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
+	if (!nlk->groups)
+		return 0;
+
+	if (!groups && !(u32)nlk->groups[0])
 		return 0;
 
 	netlink_table_grab();
-- 
1.9.1

Re: [PATCH v2] netlink: fix null pointer dereference on nlk->groups

[David Miller]

From: Baozeng Ding <sploving1@gmail.com>
Date: Sat,  9 Jan 2016 23:56:41 +0800

> If groups is not 0 and nlk->groups is NULL, it will not return
> immediately and cause a null pointer dereference later.
> 
> Signed-off-by: Baozeng Ding <sploving1@gmail.com>
> ---
> This version uses the preferred networking coding style. Thanks
> for Sergei's feedback. Also the patch keeps the original author's
> coding style as much as possible.

Is this an actual legal state?  If not, add a WARN_ON() check.

Otherwise, provide a proper OOPS log and explain how the state
can be achieved.

Thanks.

[PATCH v3] netlink: fix null pointer dereference on nlk->groups

[Baozeng Ding]

If groups is not 0 and nlk->groups is NULL, it will not return
immediately and cause a null pointer dereference later.

Signed-off-by: Baozeng Ding <sploving1@gmail.com>
---
The v3 version adds WARN_ON, suggested by David Miller. Thanks for
David's feedback.
---
 net/netlink/af_netlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 59651af..f93d579 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1576,7 +1576,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 		}
 	}
 
-	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
+	if (WARN_ON(!nlk->groups))
+		return 0;
+
+	if (!groups && !(u32)nlk->groups[0])
 		return 0;
 
 	netlink_table_grab();
-- 
1.9.1

Re: [PATCH v3] netlink: fix null pointer dereference on nlk->groups

[Denis Kirjanov]

On 1/12/16, Baozeng Ding <sploving1@gmail.com> wrote:
> If groups is not 0 and nlk->groups is NULL, it will not return
> immediately and cause a null pointer dereference later.
>
> Signed-off-by: Baozeng Ding <sploving1@gmail.com>
> ---
> The v3 version adds WARN_ON, suggested by David Miller. Thanks for
> David's feedback.

But can you explain how can we get to this situation?

> ---
>  net/netlink/af_netlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 59651af..f93d579 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1576,7 +1576,10 @@ static int netlink_bind(struct socket *sock, struct
> sockaddr *addr,
>  		}
>  	}
>
> -	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> +	if (WARN_ON(!nlk->groups))
> +		return 0;
> +
> +	if (!groups && !(u32)nlk->groups[0])
>  		return 0;
>
>  	netlink_table_grab();
> --
> 1.9.1
>
>

Re: [PATCH v3] netlink: fix null pointer dereference on nlk->groups

[Herbert Xu]

Denis Kirjanov <kda@linux-powerpc.org> wrote:
> On 1/12/16, Baozeng Ding <sploving1@gmail.com> wrote:
>> If groups is not 0 and nlk->groups is NULL, it will not return
>> immediately and cause a null pointer dereference later.
>>
>> Signed-off-by: Baozeng Ding <sploving1@gmail.com>
>> ---
>> The v3 version adds WARN_ON, suggested by David Miller. Thanks for
>> David's feedback.
> 
> But can you explain how can we get to this situation?

Indeed, this patch makes no sense as it stands.  If groups is
not NULL, then we should have allocated nlk->groups prior to
reaching this point.  If that allocation failed, then we should
have already bailed out long ago.  Perhaps we have a race condition
that needs to be closed with a lock?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] netlink: fix null pointer dereference on nlk->groups

[David Miller]

From: Baozeng Ding <sploving1@gmail.com>
Date: Tue, 12 Jan 2016 19:10:43 +0800

> If groups is not 0 and nlk->groups is NULL, it will not return
> immediately and cause a null pointer dereference later.
> 
> Signed-off-by: Baozeng Ding <sploving1@gmail.com>
> ---
> The v3 version adds WARN_ON, suggested by David Miller. Thanks for
> David's feedback.

My feedback was also that the situation is illegal, and therefore you
have to explain to us how this can actually occur.