* [HELP] ATM: mpc, use-after-free
@ 2010-10-11 7:56 Jiri Slaby
2010-10-11 8:18 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2010-10-11 7:56 UTC (permalink / raw)
To: David S. Miller; +Cc: ML netdev, linux-atm-general, LKML, chas
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,
--
js
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [HELP] ATM: mpc, use-after-free
2010-10-11 7:56 [HELP] ATM: mpc, use-after-free Jiri Slaby
@ 2010-10-11 8:18 ` Eric Dumazet
2010-10-11 8:46 ` [PATCH 1/1] ATM: mpc, fix use after free Jiri Slaby
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2010-10-11 8:18 UTC (permalink / raw)
To: Jiri Slaby; +Cc: David S. Miller, ML netdev, linux-atm-general, LKML, chas
Le lundi 11 octobre 2010 à 09:56 +0200, Jiri Slaby a écrit :
> 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?
Yes
- memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
+ memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/1] ATM: mpc, fix use after free
2010-10-11 8:18 ` Eric Dumazet
@ 2010-10-11 8:46 ` Jiri Slaby
2010-10-11 8:59 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2010-10-11 8:46 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-atm-general, linux-kernel, jirislaby, Eric Dumazet
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 <jslaby@suse.cz>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
net/atm/mpc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index 622b471..74bcc66 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -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);
}
--
1.7.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] ATM: mpc, fix use after free
2010-10-11 8:46 ` [PATCH 1/1] ATM: mpc, fix use after free Jiri Slaby
@ 2010-10-11 8:59 ` Eric Dumazet
2010-10-11 18:12 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2010-10-11 8:59 UTC (permalink / raw)
To: Jiri Slaby; +Cc: davem, netdev, linux-atm-general, linux-kernel, jirislaby
Le lundi 11 octobre 2010 à 10:46 +0200, Jiri Slaby a écrit :
> 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 <jslaby@suse.cz>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/atm/mpc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> index 622b471..74bcc66 100644
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -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 <eric.dumazet@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] ATM: mpc, fix use after free
2010-10-11 8:59 ` Eric Dumazet
@ 2010-10-11 18:12 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2010-10-11 18:12 UTC (permalink / raw)
To: eric.dumazet; +Cc: jslaby, netdev, linux-atm-general, linux-kernel, jirislaby
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 10:59:40 +0200
> Le lundi 11 octobre 2010 à 10:46 +0200, Jiri Slaby a écrit :
>> 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 <jslaby@suse.cz>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
...
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-10-11 18:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-11 7:56 [HELP] ATM: mpc, use-after-free Jiri Slaby
2010-10-11 8:18 ` Eric Dumazet
2010-10-11 8:46 ` [PATCH 1/1] ATM: mpc, fix use after free Jiri Slaby
2010-10-11 8:59 ` Eric Dumazet
2010-10-11 18:12 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).