Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752468AbdGXBac (ORCPT ); Sun, 23 Jul 2017 21:30:32 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:10338 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751839AbdGXBaZ (ORCPT ); Sun, 23 Jul 2017 21:30:25 -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> <8f6c2fec-0ac4-1414-69da-462114a4481f@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: Date: Mon, 24 Jul 2017 09:28:45 +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: <8f6c2fec-0ac4-1414-69da-462114a4481f@huawei.com> 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.0A020205.59754DA7.006F,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: 2015 Lines: 65 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) 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); > > >> > > > . >