netdev - Re: [Patch net] packet: fix 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] [day] [month] [year] [list]

Message-Id: <20170724.173339.1901009068070082994.davem@davemloft.net>
Date:   Mon, 24 Jul 2017 17:33:39 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     xiyou.wangcong@...il.com
Cc:     netdev@...r.kernel.org, alexander.levin@...izon.com,
        davej@...emonkey.org.uk, liujian56@...wei.com,
        dingtianhong@...wei.com, willemdebruijn.kernel@...il.com
Subject: Re: [Patch net] packet: fix use-after-free in
 prb_retire_rx_blk_timer_expired()

From: Cong Wang <xiyou.wangcong@...il.com>
Date: Mon, 24 Jul 2017 10:07:32 -0700

> There are multiple reports showing we have a use-after-free in
> the timer prb_retire_rx_blk_timer_expired(), where we use struct
> tpacket_kbdq_core::pkbdq, a pg_vec, after it gets freed by
> free_pg_vec().
> 
> The interesting part is it is not freed via packet_release() but
> via packet_setsockopt(), which means we are not closing the socket.
> Looking into the big and fat function packet_set_ring(), this could
> happen if we satisfy the following conditions:
> 
> 1. closing == 0, not on packet_release() path
> 2. req->tp_block_nr == 0, we don't allocate a new pg_vec
> 3. rx_ring->pg_vec is already set as V3, which means we already called
>    packet_set_ring() wtih req->tp_block_nr > 0 previously
> 4. req->tp_frame_nr == 0, pass sanity check
> 5. po->mapped == 0, never called mmap()
> 
> In this scenario we are clearing the old rx_ring->pg_vec, so we need
> to free this pg_vec, but we don't stop the timer on this path because
> of closing==0.
> 
> The timer has to be stopped as long as we need to free pg_vec, therefore
> the check on closing!=0 is wrong, we should check pg_vec!=NULL instead.
> 
> Thanks to liujian for testing different fixes.
> 
> Reported-by: alexander.levin@...izon.com
> Reported-by: Dave Jones <davej@...emonkey.org.uk>
> Reported-by: liujian (CE) <liujian56@...wei.com>
> Tested-by: liujian (CE) <liujian56@...wei.com>
> Cc: Ding Tianhong <dingtianhong@...wei.com>
> Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>

Applied and queued up for -stable, thanks!