Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752365AbdGXBO3 (ORCPT ); Sun, 23 Jul 2017 21:14:29 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:10337 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752230AbdGXBOV (ORCPT ); Sun, 23 Jul 2017 21:14:21 -0400 Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired To: Cong Wang , "liujian (CE)" References: <20170410190350.ngfw435zzr7gpw7e@sasha-lappy> <20170410192309.35x7ddya2cyyv4y6@codemonkey.org.uk> <4F88C5DDA1E80143B232E89585ACE27D018C33F1@DGGEMA502-MBX.china.huawei.com> <4F88C5DDA1E80143B232E89585ACE27D018CB6E0@DGGEMA502-MBS.china.huawei.com> CC: Willem de Bruijn , Dave Jones , "alexander.levin@verizon.com" , "davem@davemloft.net" , "edumazet@google.com" , "willemb@google.com" , "daniel@iogearbox.net" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" From: Ding Tianhong Message-ID: <8f6c2fec-0ac4-1414-69da-462114a4481f@huawei.com> Date: Mon, 24 Jul 2017 09:09:58 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.23.32] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020203.59754925.003E,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: be7c6a740eb626ce6b45c0a4769682d1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1811 Lines: 54 On 2017/7/24 1:03, Cong Wang wrote: > On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) 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 . 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); >