Skip to contentSkip to conversation

ATM: mpc, use-after-free

HELP Applied Jiri Slaby 4 participants 5 messages Oct 11, 2010

Subsystems (3)
Networking Network drivers ATM
lore
Share & tools
lore

Jiri Slaby

author
#0 lore raw

Hi,

Stanse found this use-after-free:

static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb) {
...

        new_skb = skb_realloc_headroom(skb, eg->ctrl_info.DH_length);

        dev_kfree_skb_any(skb);

FREE ^^^^^^^^^^^^^^^^^^^^^^^

        if (new_skb == NULL) {
                mpc->eg_ops->put(eg);
                return;
        }
        skb_push(new_skb, eg->ctrl_info.DH_length);
        skb_copy_to_linear_data(new_skb, eg->ctrl_info.DLL_header,
                                eg->ctrl_info.DH_length);
 ...
        memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));

USE ^^^^^^^^^^^^

        netif_rx(new_skb);

I guess it should be ATM_SKB(new_skb), right?

The two problems are:
1) obvious use-after-free
2) ?data leak, since we don't erase the right memory?

thanks,

Signature · 1 line
js
#1 lore raw
Quoted 23 lines · Jiri Slaby

Hi,

Stanse found this use-after-free:

static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb) {
...

        new_skb = skb_realloc_headroom(skb, eg->ctrl_info.DH_length);

        dev_kfree_skb_any(skb);

FREE ^^^^^^^^^^^^^^^^^^^^^^^

        if (new_skb == NULL) {
                mpc->eg_ops->put(eg);
                return;
        }
        skb_push(new_skb, eg->ctrl_info.DH_length);
        skb_copy_to_linear_data(new_skb, eg->ctrl_info.DLL_header,
                                eg->ctrl_info.DH_length);
 ...
        memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
Quoted 5 lines

USE ^^^^^^^^^^^^

        netif_rx(new_skb);

I guess it should be ATM_SKB(new_skb), right?

Yes

- memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
+ memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));

Quoted 6 lines

The two problems are:
1) obvious use-after-free
2) ?data leak, since we don't erase the right memory?

thanks,

Indeed, please submit a formal patch ?

Thanks

#2 lore raw

PATCH 1/1 ATM: mpc, fix use after free

Stanse found that mpc_push frees skb and then it dereferences it. It is a typo, new_skb should be dereferenced there.

Signed-off-by Jiri Slaby <[email protected]>
Cc Eric Dumazet <[email protected]>
---
 net/atm/mpc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/atm/mpc.c +1 -1
index 622b471..74bcc66 100644
@@ -778,7 +778,7 @@ static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb)
 	eg->packets_rcvd++;
 	mpc->eg_ops->put(eg);
 
-	memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
+	memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));
 	netif_rx(new_skb);
 }
Signature · 1 line
1.7.3.1
Acked-by on v1 1/1
#3 lore raw

Re: [PATCH 1/1] ATM: mpc, fix use after free

Quoted 14 lines · Jiri Slaby

Stanse found that mpc_push frees skb and then it dereferences it. It is a typo, new_skb should be dereferenced there.

Signed-off-by Jiri Slaby <[email protected]>
Cc Eric Dumazet <[email protected]>
---
 net/atm/mpc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/atm/mpc.c +0 -0
index 622b471..74bcc66 100644

net/atm/mpc.c static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb) · line 784

Quoted 8 lines
net/atm/mpc.c +1 -1
@@ -778,7 +778,7 @@ static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb)
 	eg->packets_rcvd++;
 	mpc->eg_ops->put(eg);
 
-	memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
+	memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));
 	netif_rx(new_skb);
 }
Acked-by Eric Dumazet <[email protected]>