netdev - Re: af_packet: use after free in prb_retire_rx_blk_timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b1e9ee12-1f81-28ea-e872-d1133ce37288@huawei.com>
Date:   Mon, 24 Jul 2017 09:28:45 +0800
From:   Ding Tianhong <dingtianhong@...wei.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        "liujian (CE)" <liujian56@...wei.com>
CC:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Dave Jones <davej@...emonkey.org.uk>,
        "alexander.levin@...izon.com" <alexander.levin@...izon.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "willemb@...gle.com" <willemb@...gle.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired



On 2017/7/24 9:09, Ding Tianhong wrote:
> 
> 
> On 2017/7/24 1:03, Cong Wang wrote:
>> On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) <liujian56@...wei.com> wrote:
>>> Hi
>>>
>>> I find it caused by below steps:
>>> 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1
>>> 2. set tp_block_nr to 0
>>> Then pg_vec was freed, and we did not delete the timer?
>>
>> Thanks for testing!
>>
>> Ah, I overlook the initialization case in my previous patch.
>>
>> How about the following one? Does it cover all the cases?
>>
>>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 008bb34ee324..0615c2a950fa 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk,
>> union tpacket_req_u *req_u,
>>                 register_prot_hook(sk);
>>         }
>>         spin_unlock(&po->bind_lock);
>> -       if (closing && (po->tp_version > TPACKET_V2)) {
>> +       if (pg_vec && (po->tp_version > TPACKET_V2)) {
>>                 /* Because we don't support block-based V3 on tx-ring */
>>                 if (!tx_ring)
>>                         prb_shutdown_retire_blk_timer(po, rb_queue);
>>
>> .
> 
> Hi, Cong:
> 
> It looks like could not cover the case: req->tp_block_nr = 2 -> reg->tp_block_nr = 1 .
> 

Oh, looks like this case would never happen, so I think your solution is ok.

> what about this way:
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4331,13 +4331,17 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>                 register_prot_hook(sk);
>         }
>         spin_unlock(&po->bind_lock);
> -       if (closing && (po->tp_version > TPACKET_V2)) {
> +       if ((closing || (pg_vec && !reg->tp_block_nr))&& (po->tp_version > TPACKET_V2)) {
>                 /* Because we don't support block-based V3 on tx-ring */
>                 if (!tx_ring)
>                         prb_shutdown_retire_blk_timer(po, rb_queue);
> 
> 

>>
> 
> 
> .
>