Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1694139 > unrolled thread

RE: af_packet: use after free in prb_retire_rx_blk_timer_expired

Started by"liujian (CE)" <liujian56@huawei.com>
First post2017-07-22 12:10 +0200
Last post2017-07-24 12:50 +0200
Articles 11 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: af_packet: use after free in prb_retire_rx_blk_timer_expired "liujian (CE)" <liujian56@huawei.com> - 2017-07-22 12:10 +0200
    Re: af_packet: use after free in prb_retire_rx_blk_timer_expired Cong Wang <xiyou.wangcong@gmail.com> - 2017-07-22 21:10 +0200
      Re: af_packet: use after free in prb_retire_rx_blk_timer_expired Ding Tianhong <dingtianhong@huawei.com> - 2017-07-23 05:50 +0200
        Re: af_packet: use after free in prb_retire_rx_blk_timer_expired Cong Wang <xiyou.wangcong@gmail.com> - 2017-07-23 08:00 +0200
          RE: af_packet: use after free in prb_retire_rx_blk_timer_expired "liujian (CE)" <liujian56@huawei.com> - 2017-07-23 10:30 +0200
          RE: af_packet: use after free in prb_retire_rx_blk_timer_expired "liujian (CE)" <liujian56@huawei.com> - 2017-07-23 12:00 +0200
          RE: af_packet: use after free in prb_retire_rx_blk_timer_expired "liujian (CE)" <liujian56@huawei.com> - 2017-07-23 15:10 +0200
            Re: af_packet: use after free in prb_retire_rx_blk_timer_expired Cong Wang <xiyou.wangcong@gmail.com> - 2017-07-23 19:10 +0200
              Re: af_packet: use after free in prb_retire_rx_blk_timer_expired Ding Tianhong <dingtianhong@huawei.com> - 2017-07-24 03:20 +0200
                Re: af_packet: use after free in prb_retire_rx_blk_timer_expired Ding Tianhong <dingtianhong@huawei.com> - 2017-07-24 03:40 +0200
                  RE: af_packet: use after free in prb_retire_rx_blk_timer_expired "liujian (CE)" <liujian56@huawei.com> - 2017-07-24 12:50 +0200

#1694139 — RE: af_packet: use after free in prb_retire_rx_blk_timer_expired

From"liujian (CE)" <liujian56@huawei.com>
Date2017-07-22 12:10 +0200
SubjectRE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Message-ID<u5Zwd-Ca-7@gated-at.bofh.it>
I also hit this issue with trinity test:

The call trace:
  [exception RIP: prb_retire_rx_blk_timer_expired+70]
    RIP: ffffffff81633be6  RSP: ffff8801bec03dc0  RFLAGS: 00010246
    RAX: 0000000000000000  RBX: ffff8801b49d0948  RCX: 0000000000000000
    RDX: ffff8801b31057a0  RSI: a56b6b6b6b6b6b6b  RDI: ffff8801b49d09ec
    RBP: ffff8801bec03dd8   R8: 0000000000000001   R9: ffffffff83e1bf80
    R10: 0000000000000002  R11: 0000000000000005  R12: ffff8801b49d09ec
    R13: 0000000000000100  R14: ffffffff81633ba0  R15: ffff8801b49d0948
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
 #7 [ffff8801bec03de0] call_timer_fn at ffffffff8108cb76
 #8 [ffff8801bec03e18] run_timer_softirq at ffffffff8108f87c
 #9 [ffff8801bec03e90] __do_softirq at ffffffff8108629f
#10 [ffff8801bec03f00] call_softirq at ffffffff8166a01c
#11 [ffff8801bec03f18] do_softirq at ffffffff810172ad
#12 [ffff8801bec03f30] irq_exit at ffffffff81086655
#13 [ffff8801bec03f48] msa_irq_exit at ffffffff810b1ab3
#14 [ffff8801bec03f88] smp_apic_timer_interrupt at ffffffff8166aeae
#15 [ffff8801bec03fb0] apic_timer_interrupt at ffffffff816692dd
--- <IRQ stack> ---

And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); is a56b6b6b6b6b6b6b 


struct packet_ring_buffer  rx_ring = {
    pg_vec = 0x0, 
    head = 0x0, 
    frames_per_block = 0x400, 
    frame_size = 0x0, 
    frame_max = 0xffffffff, 
    pg_vec_order = 0x0, 
    pg_vec_pages = 0x0, 
    pg_vec_len = 0x0, 
    pending_refcnt = 0x0, 
    prb_bdqc = {
      pkbdq = 0xffff8801b31057a0, 
      feature_req_word = 0x1, 
      hdrlen = 0x44, 
      reset_pending_on_curr_blk = 0x1, 
      delete_blk_timer = 0x0, 
      kactive_blk_num = 0x0, 
      blk_sizeof_priv = 0x0, 
      last_kactive_blk_num = 0x0, 
      pkblk_start = 0xffff8800a7000000 struct: page excluded: kernel virtual address: ffff8800a7000000  type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: ffff8800a7000000  type: "gdb_readmem_callback"
<Address 0xffff8800a7000000 out of bounds>, 
      pkblk_end = 0xffff8800a7200000 "\002", 
      kblk_size = 0x200000, 
      max_frame_len = 0x1fffd0, 
      knum_blocks = 0x1, 
      knxt_seq_num = 0x2, 
      prev = 0xffff8800a7000030 struct: page excluded: kernel virtual address: ffff8800a7000030  type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: ffff8800a7000030  type: "gdb_readmem_callback"
<Address 0xffff8800a7000030 out of bounds>, 
      nxt_offset = 0xffff8800a7000030 struct: page excluded: kernel virtual address: ffff8800a7000030  type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: ffff8800a7000030  type: "gdb_readmem_callback"
<Address 0xffff8800a7000030 out of bounds>, 
      skb = 0x0, 
      blk_fill_in_prog = {
        counter = 0x0

crash> struct pgv 0xffff8801b31057a0
struct pgv {
  buffer = 0xa56b6b6b6b6b6b6b <Address 0xa56b6b6b6b6b6b6b out of bounds>
}


Best Regards,
liujian


> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Willem de Bruijn
> Sent: Wednesday, April 12, 2017 7:23 AM
> To: 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
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
> 
> On Mon, Apr 10, 2017 at 3:23 PM, Dave Jones <davej@codemonkey.org.uk>
> wrote:
> > On Mon, Apr 10, 2017 at 07:03:30PM +0000, alexander.levin@verizon.com
> wrote:
> >  > Hi all,
> >  >
> >  > I seem to be hitting this use-after-free on a -next kernel using trinity:
> >  >
> >  > [  531.036054] BUG: KASAN: use-after-free in
> > prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688)
> 
> The retire_blk_timer is called after the pg_vec struct for this ring was freed.
> This should not happen. packet_set_ring stops the timer with del_timer_sync
> when tearing down the ring before freeing that
> struct:
> 
>         if (closing && (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);
>         }
> 
>         if (pg_vec)
>                 free_pg_vec(pg_vec, order, req->tp_block_nr);
> 
> This is a similar race to the use-after-free fixed by 84ac7260236a
> ("packet: fix race condition in packet_set_ring"). The previous race was
> triggered by a call to setsockopt PACKET_VERSION changing tp_version while
> the ring is active. It is not immediately obvious what is the cause now. I
> suppose trinity does not give a trace of such system calls on this file descriptor?
> That would be helpful.
> 
> The bug report shows both a timer firing after the packet_set_ring call that
> freed the pg_vec, and later a CONFIG_DEBUG_OBJECTS_FREE warning that
> the timer is still active when the socket is closed on release of the last file
> descriptor.

[toc] | [next] | [standalone]

#1694190

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-07-22 21:10 +0200
Message-ID<u67WN-5Nu-3@gated-at.bofh.it>
In reply to#1694139
Hello,

On Sat, Jul 22, 2017 at 2:55 AM, liujian (CE) <liujian56@huawei.com> wrote:
> I also hit this issue with trinity test:
>
> The call trace:
>   [exception RIP: prb_retire_rx_blk_timer_expired+70]
>     RIP: ffffffff81633be6  RSP: ffff8801bec03dc0  RFLAGS: 00010246
>     RAX: 0000000000000000  RBX: ffff8801b49d0948  RCX: 0000000000000000
>     RDX: ffff8801b31057a0  RSI: a56b6b6b6b6b6b6b  RDI: ffff8801b49d09ec
>     RBP: ffff8801bec03dd8   R8: 0000000000000001   R9: ffffffff83e1bf80
>     R10: 0000000000000002  R11: 0000000000000005  R12: ffff8801b49d09ec
>     R13: 0000000000000100  R14: ffffffff81633ba0  R15: ffff8801b49d0948
>     ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
>  #7 [ffff8801bec03de0] call_timer_fn at ffffffff8108cb76
>  #8 [ffff8801bec03e18] run_timer_softirq at ffffffff8108f87c
>  #9 [ffff8801bec03e90] __do_softirq at ffffffff8108629f
> #10 [ffff8801bec03f00] call_softirq at ffffffff8166a01c
> #11 [ffff8801bec03f18] do_softirq at ffffffff810172ad
> #12 [ffff8801bec03f30] irq_exit at ffffffff81086655
> #13 [ffff8801bec03f48] msa_irq_exit at ffffffff810b1ab3
> #14 [ffff8801bec03f88] smp_apic_timer_interrupt at ffffffff8166aeae
> #15 [ffff8801bec03fb0] apic_timer_interrupt at ffffffff816692dd
> --- <IRQ stack> ---
>
> And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); is a56b6b6b6b6b6b6b
>

Does the following quick fix help?


diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 008bb34ee324..09ec1640e5f7 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -4264,6 +4264,7 @@ static int packet_set_ring(struct sock *sk,
union tpacket_req_u *req_u,
                        /* Block transmit is not supported yet */
                        if (!tx_ring) {
                                init_prb_bdqc(po, rb, pg_vec, req_u);
+                               pg_vec = NULL;
                        } else {
                                struct tpacket_req3 *req3 = &req_u->req3;

[toc] | [prev] | [next] | [standalone]

#1694244

FromDing Tianhong <dingtianhong@huawei.com>
Date2017-07-23 05:50 +0200
Message-ID<u6g41-2gT-1@gated-at.bofh.it>
In reply to#1694190

On 2017/7/23 3:02, Cong Wang wrote:
> Hello,
> 
> On Sat, Jul 22, 2017 at 2:55 AM, liujian (CE) <liujian56@huawei.com> wrote:
>> I also hit this issue with trinity test:
>>
>> The call trace:
>>   [exception RIP: prb_retire_rx_blk_timer_expired+70]
>>     RIP: ffffffff81633be6  RSP: ffff8801bec03dc0  RFLAGS: 00010246
>>     RAX: 0000000000000000  RBX: ffff8801b49d0948  RCX: 0000000000000000
>>     RDX: ffff8801b31057a0  RSI: a56b6b6b6b6b6b6b  RDI: ffff8801b49d09ec
>>     RBP: ffff8801bec03dd8   R8: 0000000000000001   R9: ffffffff83e1bf80
>>     R10: 0000000000000002  R11: 0000000000000005  R12: ffff8801b49d09ec
>>     R13: 0000000000000100  R14: ffffffff81633ba0  R15: ffff8801b49d0948
>>     ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
>>  #7 [ffff8801bec03de0] call_timer_fn at ffffffff8108cb76
>>  #8 [ffff8801bec03e18] run_timer_softirq at ffffffff8108f87c
>>  #9 [ffff8801bec03e90] __do_softirq at ffffffff8108629f
>> #10 [ffff8801bec03f00] call_softirq at ffffffff8166a01c
>> #11 [ffff8801bec03f18] do_softirq at ffffffff810172ad
>> #12 [ffff8801bec03f30] irq_exit at ffffffff81086655
>> #13 [ffff8801bec03f48] msa_irq_exit at ffffffff810b1ab3
>> #14 [ffff8801bec03f88] smp_apic_timer_interrupt at ffffffff8166aeae
>> #15 [ffff8801bec03fb0] apic_timer_interrupt at ffffffff816692dd
>> --- <IRQ stack> ---
>>
>> And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); is a56b6b6b6b6b6b6b
>>
> 
> Does the following quick fix help?
> 
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 008bb34ee324..09ec1640e5f7 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4264,6 +4264,7 @@ static int packet_set_ring(struct sock *sk,
> union tpacket_req_u *req_u,
>                         /* Block transmit is not supported yet */
>                         if (!tx_ring) {
>                                 init_prb_bdqc(po, rb, pg_vec, req_u);
> +                               pg_vec = NULL;
>                         } else {
>                                 struct tpacket_req3 *req3 = &req_u->req3;
> 

Hi, Cong:

Thanks for your quirk solution, but I still has some doubts about it,
it looks like fix the problem in the packet_setsockopt->packet_set_ring processing,
but when in packet_release processing, it may could not release the
real pg_vec for the TPACKET_V3 ring, and then cause the mem leak,
maybe I miss something here, nice to hear from your feedback. :)

what about fix it this way:
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -4335,9 +4335,13 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
                /* Because we don't support block-based V3 on tx-ring */
                if (!tx_ring)
                        prb_shutdown_retire_blk_timer(po, rb_queue);
+
+               if (pg_vec)
+                       free_pg_vec(pg_vec, order, req->tp_block_nr);
+
        }

-       if (pg_vec)
+       if (pg_vec && (po->tp_version < TPACKET_V3))
                free_pg_vec(pg_vec, order, req->tp_block_nr);
 out:
        release_sock(sk);


Regards
Ding

> .
>

[toc] | [prev] | [next] | [standalone]

#1694253

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-07-23 08:00 +0200
Message-ID<u6i5Q-3vn-3@gated-at.bofh.it>
In reply to#1694244
On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong <dingtianhong@huawei.com> wrote:
> Hi, Cong:
>
> Thanks for your quirk solution, but I still has some doubts about it,
> it looks like fix the problem in the packet_setsockopt->packet_set_ring processing,
> but when in packet_release processing, it may could not release the
> real pg_vec for the TPACKET_V3 ring, and then cause the mem leak,
> maybe I miss something here, nice to hear from your feedback. :)

Yes you miss that packet_release() has memset()'s so we won't hit
that path. :)

However, I missed the swap() in this messy function, actually I
believe the bug is that we modify tpacket_kbdq_core inside rx_ring
in non-closing case without actually stopping its timer. I feel
more confident with the following patch:


diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 008bb34ee324..267b181fef15 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk,
union tpacket_req_u *req_u,
                case TPACKET_V3:
                        /* Block transmit is not supported yet */
                        if (!tx_ring) {
+                               prb_shutdown_retire_blk_timer(po, rb_queue);
                                init_prb_bdqc(po, rb, pg_vec, req_u);
                        } else {
                                struct tpacket_req3 *req3 = &req_u->req3;

[toc] | [prev] | [next] | [standalone]

#1694262

From"liujian (CE)" <liujian56@huawei.com>
Date2017-07-23 10:30 +0200
Message-ID<u6kr0-5de-5@gated-at.bofh.it>
In reply to#1694253
Hi Wang Cong,

With this patch , the system was crashed when setsockopt. 

The call trace as below:  

crash> bt
PID: 3069   TASK: ffff8800afcc0000  CPU: 0   COMMAND: "trinity-main"
 #0 [ffff8801bec03ce0] machine_kexec at ffffffff8105354b
 #1 [ffff8801bec03d40] crash_kexec at ffffffff810f7e82
 #2 [ffff8801bec03e10] panic at ffffffff81650058
 #3 [ffff8801bec03e90] watchdog_timer_fn at ffffffff81122533
 #4 [ffff8801bec03ec8] __hrtimer_run_queues at ffffffff810abeb2
 #5 [ffff8801bec03f20] hrtimer_interrupt at ffffffff810ac450
 #6 [ffff8801bec03f70] local_apic_timer_interrupt at ffffffff8104a457
 #7 [ffff8801bec03f88] smp_apic_timer_interrupt at ffffffff8166aed0
 #8 [ffff8801bec03fb0] apic_timer_interrupt at ffffffff8166931d
--- <IRQ stack> ---
 #9 [ffff8801b301fcb8] apic_timer_interrupt at ffffffff8166931d
    [exception RIP: lock_timer_base+77]
    RIP: ffffffff8108dced  RSP: ffff8801b301fd60  RFLAGS: 00000246
    RAX: 0000000000000000  RBX: ffff8800afcc0000  RCX: 0000000000000001
    RDX: ffff8800afcc0000  RSI: ffff8801b301fd90  RDI: ffff8800b0d853c8
    RBP: ffff8801b301fd80   R8: ffff8800afcc0000   R9: ffffea0002680000
    R10: 000000000000003c  R11: ffff8801b301fb2e  R12: ffff8800afcc0000
    R13: ffff8800afcc0000  R14: 0000000000000000  R15: ffffffff83d1a340
    ORIG_RAX: ffffffffffffff10  CS: 0010  SS: 0018
#10 [ffff8801b301fd88] try_to_del_timer_sync at ffffffff8108f19f
#11 [ffff8801b301fdb8] del_timer_sync at ffffffff8108f252
#12 [ffff8801b301fdd0] packet_set_ring at ffffffff81635e60
#13 [ffff8801b301fe98] packet_setsockopt at ffffffff81636760
#14 [ffff8801b301ff38] sys_setsockopt at ffffffff81531860
#15 [ffff8801b301ff80] tracesys at ffffffff816687ed (via system_call)
    RIP: 00007fcc78b03e3a  RSP: 00007fff16f246b8  RFLAGS: 00000202
    RAX: ffffffffffffffda  RBX: ffffffff816687ed  RCX: ffffffffffffffff
    RDX: 0000000000000005  RSI: 0000000000000107  RDI: 0000000000000180
    RBP: 0000000000000180   R8: 000000000000001c   R9: 00007fcc78dc7160
    R10: 0000000001fd6ba0  R11: 0000000000000202  R12: 0000000000000000
    R13: 0000000000000011  R14: 0000000001fd6b60  R15: 0000000001fd6b70
    ORIG_RAX: 0000000000000036  CS: 0033  SS: 002b


Best Regards,
liujian


> -----Original Message-----
> From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> Sent: Sunday, July 23, 2017 1:59 PM
> To: Dingtianhong
> Cc: liujian (CE); 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
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
> 
> On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong <dingtianhong@huawei.com>
> wrote:
> > Hi, Cong:
> >
> > Thanks for your quirk solution, but I still has some doubts about it,
> > it looks like fix the problem in the
> > packet_setsockopt->packet_set_ring processing, but when in
> > packet_release processing, it may could not release the real pg_vec
> > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss
> > something here, nice to hear from your feedback. :)
> 
> Yes you miss that packet_release() has memset()'s so we won't hit that path. :)
> 
> However, I missed the swap() in this messy function, actually I believe the bug
> is that we modify tpacket_kbdq_core inside rx_ring in non-closing case without
> actually stopping its timer. I feel more confident with the following patch:
> 
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> 008bb34ee324..267b181fef15 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk, union
> tpacket_req_u *req_u,
>                 case TPACKET_V3:
>                         /* Block transmit is not supported yet */
>                         if (!tx_ring) {
> +                               prb_shutdown_retire_blk_timer(po,
> + rb_queue);
>                                 init_prb_bdqc(po, rb, pg_vec, req_u);
>                         } else {
>                                 struct tpacket_req3 *req3 =
> &req_u->req3;

[toc] | [prev] | [next] | [standalone]

#1694268

From"liujian (CE)" <liujian56@huawei.com>
Date2017-07-23 12:00 +0200
Message-ID<u6lQ5-5VE-3@gated-at.bofh.it>
In reply to#1694253
Hi, 

Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to TPACKET_V1 ?


Best Regards,
liujian


> -----Original Message-----
> From: liujian (CE)
> Sent: Sunday, July 23, 2017 4:21 PM
> To: 'Cong Wang'; Dingtianhong
> 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
> Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
> 
> Hi Wang Cong,
> 
> With this patch , the system was crashed when setsockopt.
> 
> The call trace as below:
> 
> crash> bt
> PID: 3069   TASK: ffff8800afcc0000  CPU: 0   COMMAND: "trinity-main"
>  #0 [ffff8801bec03ce0] machine_kexec at ffffffff8105354b
>  #1 [ffff8801bec03d40] crash_kexec at ffffffff810f7e82
>  #2 [ffff8801bec03e10] panic at ffffffff81650058
>  #3 [ffff8801bec03e90] watchdog_timer_fn at ffffffff81122533
>  #4 [ffff8801bec03ec8] __hrtimer_run_queues at ffffffff810abeb2
>  #5 [ffff8801bec03f20] hrtimer_interrupt at ffffffff810ac450
>  #6 [ffff8801bec03f70] local_apic_timer_interrupt at ffffffff8104a457
>  #7 [ffff8801bec03f88] smp_apic_timer_interrupt at ffffffff8166aed0
>  #8 [ffff8801bec03fb0] apic_timer_interrupt at ffffffff8166931d
> --- <IRQ stack> ---
>  #9 [ffff8801b301fcb8] apic_timer_interrupt at ffffffff8166931d
>     [exception RIP: lock_timer_base+77]
>     RIP: ffffffff8108dced  RSP: ffff8801b301fd60  RFLAGS: 00000246
>     RAX: 0000000000000000  RBX: ffff8800afcc0000  RCX:
> 0000000000000001
>     RDX: ffff8800afcc0000  RSI: ffff8801b301fd90  RDI: ffff8800b0d853c8
>     RBP: ffff8801b301fd80   R8: ffff8800afcc0000   R9: ffffea0002680000
>     R10: 000000000000003c  R11: ffff8801b301fb2e  R12: ffff8800afcc0000
>     R13: ffff8800afcc0000  R14: 0000000000000000  R15: ffffffff83d1a340
>     ORIG_RAX: ffffffffffffff10  CS: 0010  SS: 0018
> #10 [ffff8801b301fd88] try_to_del_timer_sync at ffffffff8108f19f
> #11 [ffff8801b301fdb8] del_timer_sync at ffffffff8108f252
> #12 [ffff8801b301fdd0] packet_set_ring at ffffffff81635e60
> #13 [ffff8801b301fe98] packet_setsockopt at ffffffff81636760
> #14 [ffff8801b301ff38] sys_setsockopt at ffffffff81531860
> #15 [ffff8801b301ff80] tracesys at ffffffff816687ed (via system_call)
>     RIP: 00007fcc78b03e3a  RSP: 00007fff16f246b8  RFLAGS: 00000202
>     RAX: ffffffffffffffda  RBX: ffffffff816687ed  RCX: ffffffffffffffff
>     RDX: 0000000000000005  RSI: 0000000000000107  RDI:
> 0000000000000180
>     RBP: 0000000000000180   R8: 000000000000001c   R9:
> 00007fcc78dc7160
>     R10: 0000000001fd6ba0  R11: 0000000000000202  R12:
> 0000000000000000
>     R13: 0000000000000011  R14: 0000000001fd6b60  R15:
> 0000000001fd6b70
>     ORIG_RAX: 0000000000000036  CS: 0033  SS: 002b
> 
> 
> Best Regards,
> liujian
> 
> 
> > -----Original Message-----
> > From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> > Sent: Sunday, July 23, 2017 1:59 PM
> > To: Dingtianhong
> > Cc: liujian (CE); 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
> > Subject: Re: af_packet: use after free in
> > prb_retire_rx_blk_timer_expired
> >
> > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong
> > <dingtianhong@huawei.com>
> > wrote:
> > > Hi, Cong:
> > >
> > > Thanks for your quirk solution, but I still has some doubts about
> > > it, it looks like fix the problem in the
> > > packet_setsockopt->packet_set_ring processing, but when in
> > > packet_release processing, it may could not release the real pg_vec
> > > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss
> > > something here, nice to hear from your feedback. :)
> >
> > Yes you miss that packet_release() has memset()'s so we won't hit that
> > path. :)
> >
> > However, I missed the swap() in this messy function, actually I
> > believe the bug is that we modify tpacket_kbdq_core inside rx_ring in
> > non-closing case without actually stopping its timer. I feel more confident
> with the following patch:
> >
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> > 008bb34ee324..267b181fef15 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk,
> > union tpacket_req_u *req_u,
> >                 case TPACKET_V3:
> >                         /* Block transmit is not supported yet */
> >                         if (!tx_ring) {
> > +                               prb_shutdown_retire_blk_timer(po,
> > + rb_queue);
> >                                 init_prb_bdqc(po, rb, pg_vec, req_u);
> >                         } else {
> >                                 struct tpacket_req3 *req3 =
> > &req_u->req3;

[toc] | [prev] | [next] | [standalone]

#1694307

From"liujian (CE)" <liujian56@huawei.com>
Date2017-07-23 15:10 +0200
Message-ID<u6oNX-7Vt-9@gated-at.bofh.it>
In reply to#1694253
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?

Best Regards,
liujian


> -----Original Message-----
> From: liujian (CE)
> Sent: Sunday, July 23, 2017 5:47 PM
> To: liujian (CE); 'Cong Wang'; Dingtianhong
> 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'
> Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
> 
> Hi,
> 
> Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to
> TPACKET_V1 ?
> 
> 
> Best Regards,
> liujian
> 
> 
> > -----Original Message-----
> > From: liujian (CE)
> > Sent: Sunday, July 23, 2017 4:21 PM
> > To: 'Cong Wang'; Dingtianhong
> > 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
> > Subject: RE: af_packet: use after free in
> > prb_retire_rx_blk_timer_expired
> >
> > Hi Wang Cong,
> >
> > With this patch , the system was crashed when setsockopt.
> >
> > The call trace as below:
> >
> > crash> bt
> > PID: 3069   TASK: ffff8800afcc0000  CPU: 0   COMMAND: "trinity-main"
> >  #0 [ffff8801bec03ce0] machine_kexec at ffffffff8105354b
> >  #1 [ffff8801bec03d40] crash_kexec at ffffffff810f7e82
> >  #2 [ffff8801bec03e10] panic at ffffffff81650058
> >  #3 [ffff8801bec03e90] watchdog_timer_fn at ffffffff81122533
> >  #4 [ffff8801bec03ec8] __hrtimer_run_queues at ffffffff810abeb2
> >  #5 [ffff8801bec03f20] hrtimer_interrupt at ffffffff810ac450
> >  #6 [ffff8801bec03f70] local_apic_timer_interrupt at ffffffff8104a457
> >  #7 [ffff8801bec03f88] smp_apic_timer_interrupt at ffffffff8166aed0
> >  #8 [ffff8801bec03fb0] apic_timer_interrupt at ffffffff8166931d
> > --- <IRQ stack> ---
> >  #9 [ffff8801b301fcb8] apic_timer_interrupt at ffffffff8166931d
> >     [exception RIP: lock_timer_base+77]
> >     RIP: ffffffff8108dced  RSP: ffff8801b301fd60  RFLAGS: 00000246
> >     RAX: 0000000000000000  RBX: ffff8800afcc0000  RCX:
> > 0000000000000001
> >     RDX: ffff8800afcc0000  RSI: ffff8801b301fd90  RDI: ffff8800b0d853c8
> >     RBP: ffff8801b301fd80   R8: ffff8800afcc0000   R9:
> ffffea0002680000
> >     R10: 000000000000003c  R11: ffff8801b301fb2e  R12:
> ffff8800afcc0000
> >     R13: ffff8800afcc0000  R14: 0000000000000000  R15:
> ffffffff83d1a340
> >     ORIG_RAX: ffffffffffffff10  CS: 0010  SS: 0018
> > #10 [ffff8801b301fd88] try_to_del_timer_sync at ffffffff8108f19f
> > #11 [ffff8801b301fdb8] del_timer_sync at ffffffff8108f252
> > #12 [ffff8801b301fdd0] packet_set_ring at ffffffff81635e60
> > #13 [ffff8801b301fe98] packet_setsockopt at ffffffff81636760
> > #14 [ffff8801b301ff38] sys_setsockopt at ffffffff81531860
> > #15 [ffff8801b301ff80] tracesys at ffffffff816687ed (via system_call)
> >     RIP: 00007fcc78b03e3a  RSP: 00007fff16f246b8  RFLAGS: 00000202
> >     RAX: ffffffffffffffda  RBX: ffffffff816687ed  RCX: ffffffffffffffff
> >     RDX: 0000000000000005  RSI: 0000000000000107  RDI:
> > 0000000000000180
> >     RBP: 0000000000000180   R8: 000000000000001c   R9:
> > 00007fcc78dc7160
> >     R10: 0000000001fd6ba0  R11: 0000000000000202  R12:
> > 0000000000000000
> >     R13: 0000000000000011  R14: 0000000001fd6b60  R15:
> > 0000000001fd6b70
> >     ORIG_RAX: 0000000000000036  CS: 0033  SS: 002b
> >
> >
> > Best Regards,
> > liujian
> >
> >
> > > -----Original Message-----
> > > From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> > > Sent: Sunday, July 23, 2017 1:59 PM
> > > To: Dingtianhong
> > > Cc: liujian (CE); 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
> > > Subject: Re: af_packet: use after free in
> > > prb_retire_rx_blk_timer_expired
> > >
> > > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong
> > > <dingtianhong@huawei.com>
> > > wrote:
> > > > Hi, Cong:
> > > >
> > > > Thanks for your quirk solution, but I still has some doubts about
> > > > it, it looks like fix the problem in the
> > > > packet_setsockopt->packet_set_ring processing, but when in
> > > > packet_release processing, it may could not release the real
> > > > pg_vec for the TPACKET_V3 ring, and then cause the mem leak, maybe
> > > > I miss something here, nice to hear from your feedback. :)
> > >
> > > Yes you miss that packet_release() has memset()'s so we won't hit
> > > that path. :)
> > >
> > > However, I missed the swap() in this messy function, actually I
> > > believe the bug is that we modify tpacket_kbdq_core inside rx_ring
> > > in non-closing case without actually stopping its timer. I feel more
> > > confident
> > with the following patch:
> > >
> > >
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> > > 008bb34ee324..267b181fef15 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk,
> > > union tpacket_req_u *req_u,
> > >                 case TPACKET_V3:
> > >                         /* Block transmit is not supported yet */
> > >                         if (!tx_ring) {
> > > +                               prb_shutdown_retire_blk_timer(po,
> > > + rb_queue);
> > >                                 init_prb_bdqc(po, rb, pg_vec,
> req_u);
> > >                         } else {
> > >                                 struct tpacket_req3 *req3 =
> > > &req_u->req3;

[toc] | [prev] | [next] | [standalone]

#1694335

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-07-23 19:10 +0200
Message-ID<u6sye-1RK-19@gated-at.bofh.it>
In reply to#1694307
On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) <liujian56@huawei.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);

[toc] | [prev] | [next] | [standalone]

#1694387

FromDing Tianhong <dingtianhong@huawei.com>
Date2017-07-24 03:20 +0200
Message-ID<u6Acp-6uG-7@gated-at.bofh.it>
In reply to#1694335

On 2017/7/24 1:03, Cong Wang wrote:
> On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) <liujian56@huawei.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 .

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);


>

[toc] | [prev] | [next] | [standalone]

#1694392

FromDing Tianhong <dingtianhong@huawei.com>
Date2017-07-24 03:40 +0200
Message-ID<u6AvL-6Bf-5@gated-at.bofh.it>
In reply to#1694387

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@huawei.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);
> 
> 

>>
> 
> 
> .
>

[toc] | [prev] | [next] | [standalone]

#1694627

From"liujian (CE)" <liujian56@huawei.com>
Date2017-07-24 12:50 +0200
Message-ID<u6J62-3B1-13@gated-at.bofh.it>
In reply to#1694392
Hi Wang cong,

After apply the patch, I did not hit the issue again.
Thank you~


Best Regards,
liujian

> -----Original Message-----
> From: Dingtianhong
> Sent: Monday, July 24, 2017 9:29 AM
> To: Cong Wang; liujian (CE)
> 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
> 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@huawei.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);
> >
> >
> 
> >>
> >
> >
> > .
> >

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web