netdev - Re: [PATCH net] ipv4: igmp: fix refcnt uaf issue when receiving igmp query packet

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: <CANn89i+zX5-xdXo0nezZiXS2+JXvcr-nsmaCmc8gNzuB5Xg5hQ@mail.gmail.com>
Date: Tue, 21 Nov 2023 18:37:15 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Zhengchao Shao <shaozhengchao@...wei.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, dsahern@...nel.org, 
	kuba@...nel.org, pabeni@...hat.com, weiyongjun1@...wei.com, 
	yuehaibing@...wei.com
Subject: Re: [PATCH net] ipv4: igmp: fix refcnt uaf issue when receiving igmp
 query packet

On Tue, Nov 21, 2023 at 2:53 AM Zhengchao Shao <shaozhengchao@...wei.com> wrote:
>
> When I perform the following test operations:
> 1.ip link add br0 type bridge
> 2.brctl addif br0 eth0
> 3.ip addr add 239.0.0.1/32 dev eth0
> 4.ip addr add 239.0.0.1/32 dev br0
> 5.ip addr add 224.0.0.1/32 dev br0
> 6.while ((1))
>     do
>         ifconfig br0 up
>         ifconfig br0 down
>     done
> 7.send IGMPv2 query packets to port eth0 continuously. For example,
> ./mausezahn ethX -c 0 "01 00 5e 00 00 01 00 72 19 88 aa 02 08 00 45 00 00
> 1c 00 01 00 00 01 02 0e 7f c0 a8 0a b7 e0 00 00 01 11 64 ee 9b 00 00 00 00"
>
> The preceding tests may trigger the refcnt uaf isuue of the mc list. The
> stack is as follows:
>         refcount_t: addition on 0; use-after-free.
>         WARNING: CPU: 21 PID: 144 at lib/refcount.c:25 refcount_warn_saturate+0x78/0x110
>         CPU: 21 PID: 144 Comm: ksoftirqd/21 Kdump: loaded Not tainted 6.7.0-rc1-next-20231117-dirty #57
>         RIP: 0010:refcount_warn_saturate+0x78/0x110
>         Call Trace:
>         <TASK>
>         ? __warn+0x83/0x130
>         ? refcount_warn_saturate+0x78/0x110
>         ? __report_bug+0xea/0x100
>         ? report_bug+0x24/0x70
>         ? handle_bug+0x3c/0x70
>         ? exc_invalid_op+0x18/0x70
>         igmp_heard_query+0x221/0x690
>         igmp_rcv+0xea/0x2f0
>         ip_protocol_deliver_rcu+0x156/0x160
>         ip_local_deliver_finish+0x77/0xa0
>         __netif_receive_skb_one_core+0x8b/0xa0
>         netif_receive_skb_internal+0x80/0xd0
>         netif_receive_skb+0x18/0xc0
>         br_handle_frame_finish+0x340/0x5c0 [bridge]
>         nf_hook_bridge_pre+0x117/0x130 [bridge]
>         __netif_receive_skb_core+0x241/0x1090
>         __netif_receive_skb_list_core+0x13f/0x2e0
>         __netif_receive_skb_list+0xfc/0x190
>         netif_receive_skb_list_internal+0x102/0x1e0
>         napi_gro_receive+0xd7/0x220
>         e1000_clean_rx_irq+0x1d4/0x4f0 [e1000]
>         e1000_clean+0x5e/0xe0 [e1000]
>         __napi_poll+0x2c/0x1b0
>         net_rx_action+0x2cb/0x3a0
>         __do_softirq+0xcd/0x2a7
>         run_ksoftirqd+0x22/0x30
>         smpboot_thread_fn+0xdb/0x1d0
>         kthread+0xe2/0x110
>         ret_from_fork+0x34/0x50
>         ret_from_fork_asm+0x1a/0x30
>         </TASK>


Please include symbols in stack traces, otherwise they are not precise enough.

scripts/decode_stacktrace.sh is your friend.

git grep -n scripts/decode_stacktrace.sh -- Documentation/admin-guide

>
> The root causes are as follows:
> Thread A                                        Thread B
> ...                                             netif_receive_skb
> br_dev_stop                                     ...
>     br_multicast_leave_snoopers                 ...
>         __ip_mc_dec_group                       ...
>             __igmp_group_dropped                igmp_rcv
>                 igmp_stop_timer                     igmp_heard_query         //ref = 1
>                 ip_ma_put                               igmp_mod_timer
>                     refcount_dec_and_test                   igmp_start_timer //ref = 0
>                         ...                                     refcount_inc //ref increases from 0
> When the device receives an IGMPv2 Query message, it starts the timer
> immediately, regardless of whether the device is running. If the device is
> down and has left the multicast group, it will cause the mc list refcount
> uaf issue.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.com>
> ---
>  net/ipv4/igmp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index 76c3ea75b8dd..f217581904d6 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -1044,6 +1044,8 @@ static bool igmp_heard_query(struct in_device *in_dev, struct sk_buff *skb,
>         for_each_pmc_rcu(in_dev, im) {
>                 int changed;
>
> +               if (!netif_running(im->interface->dev))
> +                       continue;

This seems racy to me.

I guess igmp_start_timer() should use refcount_inc_not_zero() instead.

>                 if (group && group != im->multiaddr)
>                         continue;
>                 if (im->multiaddr == IGMP_ALL_HOSTS)
> --
> 2.34.1
>