Patchwork [Lucid,CVE-2013-1826] xfrm_user: return error pointer instead of NULL

login
register
mail settings
Submitter Luis Henriques
Date April 17, 2013, 12:24 p.m.
Message ID <1366201495-12547-1-git-send-email-luis.henriques@canonical.com>
Download mbox | patch
Permalink /patch/237211/
State New
Headers show

Comments

Luis Henriques - April 17, 2013, 12:24 p.m.
From: Mathias Krause <minipli@googlemail.com>

CVE-2013-1826

BugLink: http://bugs.launchpad.net/bugs/1155026

When dump_one_state() returns an error, e.g. because of a too small
buffer to dump the whole xfrm state, xfrm_state_netlink() returns NULL
instead of an error pointer. But its callers expect an error pointer
and therefore continue to operate on a NULL skbuff.

This could lead to a privilege escalation (execution of user code in
kernel context) if the attacker has CAP_NET_ADMIN and is able to map
address 0.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry picked from commit 864745d291b5ba80ea0bd0edcbe67273de368836)

Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 net/xfrm/xfrm_user.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Tim Gardner - April 17, 2013, 12:55 p.m.

Colin King - April 17, 2013, 1 p.m.
On 17/04/13 13:24, Luis Henriques wrote:
> From: Mathias Krause <minipli@googlemail.com>
>
> CVE-2013-1826
>
> BugLink: http://bugs.launchpad.net/bugs/1155026
>
> When dump_one_state() returns an error, e.g. because of a too small
> buffer to dump the whole xfrm state, xfrm_state_netlink() returns NULL
> instead of an error pointer. But its callers expect an error pointer
> and therefore continue to operate on a NULL skbuff.
>
> This could lead to a privilege escalation (execution of user code in
> kernel context) if the attacker has CAP_NET_ADMIN and is able to map
> address 0.
>
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry picked from commit 864745d291b5ba80ea0bd0edcbe67273de368836)
>
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
>   net/xfrm/xfrm_user.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a8d83c4..dff20ac 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -647,6 +647,7 @@ static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb,
>   {
>   	struct xfrm_dump_info info;
>   	struct sk_buff *skb;
> +	int err;
>
>   	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>   	if (!skb)
> @@ -657,9 +658,10 @@ static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb,
>   	info.nlmsg_seq = seq;
>   	info.nlmsg_flags = 0;
>
> -	if (dump_one_state(x, 0, &info)) {
> +	err = dump_one_state(x, 0, &info);
> +	if (err) {
>   		kfree_skb(skb);
> -		return NULL;
> +		return ERR_PTR(err);
>   	}
>
>   	return skb;
>
Clean cherry pick.  All the callers of xfrm_state_netlink() do indeed
check for an error pointer rather than NULL, so this patch looks sane 
for Lucid.

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a8d83c4..dff20ac 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -647,6 +647,7 @@  static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb,
 {
 	struct xfrm_dump_info info;
 	struct sk_buff *skb;
+	int err;
 
 	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 	if (!skb)
@@ -657,9 +658,10 @@  static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb,
 	info.nlmsg_seq = seq;
 	info.nlmsg_flags = 0;
 
-	if (dump_one_state(x, 0, &info)) {
+	err = dump_one_state(x, 0, &info);
+	if (err) {
 		kfree_skb(skb);
-		return NULL;
+		return ERR_PTR(err);
 	}
 
 	return skb;