* [PATCH net] tun: fix use after free for ptr_ring
@ 2018-05-09 6:59 Jason Wang
2018-05-10 18:08 ` Cong Wang
2018-05-11 17:52 ` Michael S. Tsirkin
0 siblings, 2 replies; 4+ messages in thread
From: Jason Wang @ 2018-05-09 6:59 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: Jason Wang, Eric Dumazet, Cong Wang, Michael S . Tsirkin
We used to initialize ptr_ring during TUNSETIFF, this is because its
size depends on the tx_queue_len of netdevice. And we try to clean it
up when socket were detached from netdevice. A race were spotted when
trying to do uninit during a read which will lead a use after free for
pointer ring. Solving this by always initialize a zero size ptr_ring
in open() and do resizing during TUNSETIFF, and then we can safely do
cleanup during close(). With this, there's no need for the workaround
that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
for tfile->tx_array").
Reported-by: syzbot+e8b902c3c3fadf0a9dba@syzkaller.appspotmail.com
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/tun.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index ef33950..298cb96 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -681,15 +681,6 @@ static void tun_queue_purge(struct tun_file *tfile)
skb_queue_purge(&tfile->sk.sk_error_queue);
}
-static void tun_cleanup_tx_ring(struct tun_file *tfile)
-{
- if (tfile->tx_ring.queue) {
- ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
- xdp_rxq_info_unreg(&tfile->xdp_rxq);
- memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
- }
-}
-
static void __tun_detach(struct tun_file *tfile, bool clean)
{
struct tun_file *ntfile;
@@ -736,7 +727,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
tun->dev->reg_state == NETREG_REGISTERED)
unregister_netdevice(tun->dev);
}
- tun_cleanup_tx_ring(tfile);
+ if (tun)
+ xdp_rxq_info_unreg(&tfile->xdp_rxq);
sock_put(&tfile->sk);
}
}
@@ -783,14 +775,14 @@ static void tun_detach_all(struct net_device *dev)
tun_napi_del(tun, tfile);
/* Drop read queue */
tun_queue_purge(tfile);
+ xdp_rxq_info_unreg(&tfile->xdp_rxq);
sock_put(&tfile->sk);
- tun_cleanup_tx_ring(tfile);
}
list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
tun_enable_queue(tfile);
tun_queue_purge(tfile);
+ xdp_rxq_info_unreg(&tfile->xdp_rxq);
sock_put(&tfile->sk);
- tun_cleanup_tx_ring(tfile);
}
BUG_ON(tun->numdisabled != 0);
@@ -834,7 +826,8 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
}
if (!tfile->detached &&
- ptr_ring_init(&tfile->tx_ring, dev->tx_queue_len, GFP_KERNEL)) {
+ ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
+ GFP_KERNEL, __skb_array_destroy_skb)) {
err = -ENOMEM;
goto out;
}
@@ -3219,6 +3212,11 @@ static int tun_chr_open(struct inode *inode, struct file * file)
&tun_proto, 0);
if (!tfile)
return -ENOMEM;
+ if (ptr_ring_init(&tfile->tx_ring, 0, GFP_KERNEL)) {
+ sk_free(&tfile->sk);
+ return -ENOMEM;
+ }
+
RCU_INIT_POINTER(tfile->tun, NULL);
tfile->flags = 0;
tfile->ifindex = 0;
@@ -3239,8 +3237,6 @@ static int tun_chr_open(struct inode *inode, struct file * file)
sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
- memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
-
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] tun: fix use after free for ptr_ring
2018-05-09 6:59 [PATCH net] tun: fix use after free for ptr_ring Jason Wang
@ 2018-05-10 18:08 ` Cong Wang
2018-05-11 1:29 ` Jason Wang
2018-05-11 17:52 ` Michael S. Tsirkin
1 sibling, 1 reply; 4+ messages in thread
From: Cong Wang @ 2018-05-10 18:08 UTC (permalink / raw)
To: Jason Wang
Cc: Linux Kernel Network Developers, LKML, Eric Dumazet, Michael S . Tsirkin
On Tue, May 8, 2018 at 11:59 PM, Jason Wang <jasowang@redhat.com> wrote:
> We used to initialize ptr_ring during TUNSETIFF, this is because its
> size depends on the tx_queue_len of netdevice. And we try to clean it
> up when socket were detached from netdevice. A race were spotted when
> trying to do uninit during a read which will lead a use after free for
> pointer ring. Solving this by always initialize a zero size ptr_ring
> in open() and do resizing during TUNSETIFF, and then we can safely do
> cleanup during close(). With this, there's no need for the workaround
> that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
> for tfile->tx_array").
>
Ah, I didn't know ptr_ring_init(0) could work... Nice patch!
Except one thing below.
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index ef33950..298cb96 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -681,15 +681,6 @@ static void tun_queue_purge(struct tun_file *tfile)
> skb_queue_purge(&tfile->sk.sk_error_queue);
> }
>
> -static void tun_cleanup_tx_ring(struct tun_file *tfile)
> -{
> - if (tfile->tx_ring.queue) {
> - ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
> - xdp_rxq_info_unreg(&tfile->xdp_rxq);
> - memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
> - }
> -}
I don't think you can totally remove ptr_ring_cleanup(), it should be
called unconditionally with your ptr_ring_init(0) trick, right?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] tun: fix use after free for ptr_ring
2018-05-10 18:08 ` Cong Wang
@ 2018-05-11 1:29 ` Jason Wang
0 siblings, 0 replies; 4+ messages in thread
From: Jason Wang @ 2018-05-11 1:29 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, LKML, Eric Dumazet, Michael S . Tsirkin
On 2018年05月11日 02:08, Cong Wang wrote:
> On Tue, May 8, 2018 at 11:59 PM, Jason Wang <jasowang@redhat.com> wrote:
>> We used to initialize ptr_ring during TUNSETIFF, this is because its
>> size depends on the tx_queue_len of netdevice. And we try to clean it
>> up when socket were detached from netdevice. A race were spotted when
>> trying to do uninit during a read which will lead a use after free for
>> pointer ring. Solving this by always initialize a zero size ptr_ring
>> in open() and do resizing during TUNSETIFF, and then we can safely do
>> cleanup during close(). With this, there's no need for the workaround
>> that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
>> for tfile->tx_array").
>>
> Ah, I didn't know ptr_ring_init(0) could work... Nice patch!
> Except one thing below.
>
>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index ef33950..298cb96 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -681,15 +681,6 @@ static void tun_queue_purge(struct tun_file *tfile)
>> skb_queue_purge(&tfile->sk.sk_error_queue);
>> }
>>
>> -static void tun_cleanup_tx_ring(struct tun_file *tfile)
>> -{
>> - if (tfile->tx_ring.queue) {
>> - ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
>> - xdp_rxq_info_unreg(&tfile->xdp_rxq);
>> - memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
>> - }
>> -}
>
> I don't think you can totally remove ptr_ring_cleanup(), it should be
> called unconditionally with your ptr_ring_init(0) trick, right?
Right, my bad. Actually I do intend to cleanup it at close() like what
commit log said.
Will send v2.
Thanks
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] tun: fix use after free for ptr_ring
2018-05-09 6:59 [PATCH net] tun: fix use after free for ptr_ring Jason Wang
2018-05-10 18:08 ` Cong Wang
@ 2018-05-11 17:52 ` Michael S. Tsirkin
1 sibling, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2018-05-11 17:52 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, Eric Dumazet, Cong Wang
On Wed, May 09, 2018 at 02:59:58PM +0800, Jason Wang wrote:
> We used to initialize ptr_ring during TUNSETIFF, this is because its
> size depends on the tx_queue_len of netdevice. And we try to clean it
> up when socket were detached from netdevice. A race were spotted when
> trying to do uninit during a read which will lead a use after free for
> pointer ring. Solving this by always initialize a zero size ptr_ring
> in open() and do resizing during TUNSETIFF, and then we can safely do
> cleanup during close(). With this, there's no need for the workaround
> that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
> for tfile->tx_array").
>
> Reported-by: syzbot+e8b902c3c3fadf0a9dba@syzkaller.appspotmail.com
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/tun.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index ef33950..298cb96 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -681,15 +681,6 @@ static void tun_queue_purge(struct tun_file *tfile)
> skb_queue_purge(&tfile->sk.sk_error_queue);
> }
>
> -static void tun_cleanup_tx_ring(struct tun_file *tfile)
> -{
> - if (tfile->tx_ring.queue) {
> - ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
> - xdp_rxq_info_unreg(&tfile->xdp_rxq);
> - memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
> - }
> -}
> -
> static void __tun_detach(struct tun_file *tfile, bool clean)
> {
> struct tun_file *ntfile;
> @@ -736,7 +727,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
> tun->dev->reg_state == NETREG_REGISTERED)
> unregister_netdevice(tun->dev);
> }
> - tun_cleanup_tx_ring(tfile);
> + if (tun)
> + xdp_rxq_info_unreg(&tfile->xdp_rxq);
> sock_put(&tfile->sk);
> }
> }
> @@ -783,14 +775,14 @@ static void tun_detach_all(struct net_device *dev)
> tun_napi_del(tun, tfile);
> /* Drop read queue */
> tun_queue_purge(tfile);
> + xdp_rxq_info_unreg(&tfile->xdp_rxq);
> sock_put(&tfile->sk);
> - tun_cleanup_tx_ring(tfile);
> }
> list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
> tun_enable_queue(tfile);
> tun_queue_purge(tfile);
> + xdp_rxq_info_unreg(&tfile->xdp_rxq);
> sock_put(&tfile->sk);
> - tun_cleanup_tx_ring(tfile);
> }
> BUG_ON(tun->numdisabled != 0);
>
> @@ -834,7 +826,8 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
> }
>
> if (!tfile->detached &&
> - ptr_ring_init(&tfile->tx_ring, dev->tx_queue_len, GFP_KERNEL)) {
> + ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
> + GFP_KERNEL, __skb_array_destroy_skb)) {
> err = -ENOMEM;
> goto out;
> }
> @@ -3219,6 +3212,11 @@ static int tun_chr_open(struct inode *inode, struct file * file)
> &tun_proto, 0);
> if (!tfile)
> return -ENOMEM;
> + if (ptr_ring_init(&tfile->tx_ring, 0, GFP_KERNEL)) {
> + sk_free(&tfile->sk);
> + return -ENOMEM;
> + }
> +
> RCU_INIT_POINTER(tfile->tun, NULL);
> tfile->flags = 0;
> tfile->ifindex = 0;
> @@ -3239,8 +3237,6 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>
> sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
>
> - memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
> -
> return 0;
> }
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-11 17:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-09 6:59 [PATCH net] tun: fix use after free for ptr_ring Jason Wang
2018-05-10 18:08 ` Cong Wang
2018-05-11 1:29 ` Jason Wang
2018-05-11 17:52 ` Michael S. Tsirkin
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).