diff mbox series

[net] ipv4: igmp: fix refcnt uaf issue when receiving igmp query packet

Message ID 20231121020558.240321-1-shaozhengchao@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] ipv4: igmp: fix refcnt uaf issue when receiving igmp query packet | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1122 this patch: 1122
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 1143 this patch: 1143
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1149 this patch: 1149
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

shaozhengchao Nov. 21, 2023, 2:05 a.m. UTC
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>

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@huawei.com>
---
 net/ipv4/igmp.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Hangbin Liu Nov. 21, 2023, 2:56 a.m. UTC | #1
Hi Zhengchao,
On Tue, Nov 21, 2023 at 10:05:58AM +0800, Zhengchao Shao wrote:
> ---
>  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;

I haven't checked this part for a long time. What's the difference of in_dev->dev
and im->interface->dev? I though they are the same, no?

If they are the same, should we stop processing the query earlier? e.g.

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 76c3ea75b8dd..f4e1d229c9aa 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1082,6 +1082,9 @@ int igmp_rcv(struct sk_buff *skb)
                        goto drop;
        }

+       if (!netif_running(dev))
+               goto drop;
+
        in_dev = __in_dev_get_rcu(dev);
        if (!in_dev)
                goto drop;


BTW, does IPv6 MLD has this issue?

Thanks
Hangbin
shaozhengchao Nov. 21, 2023, 12:38 p.m. UTC | #2
On 2023/11/21 10:56, Hangbin Liu wrote:
> Hi Zhengchao,
> On Tue, Nov 21, 2023 at 10:05:58AM +0800, Zhengchao Shao wrote:
>> ---
>>   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;
> 
> I haven't checked this part for a long time. What's the difference of in_dev->dev
> and im->interface->dev? I though they are the same, no?
> 
> If they are the same, should we stop processing the query earlier? e.g.
> 

Hi Hangbin:
	Yes, they are the same.

> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index 76c3ea75b8dd..f4e1d229c9aa 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -1082,6 +1082,9 @@ int igmp_rcv(struct sk_buff *skb)
>                          goto drop;
>          }
> 
> +       if (!netif_running(dev))
> +               goto drop;
> +
>          in_dev = __in_dev_get_rcu(dev);
>          if (!in_dev)
>                  goto drop;
> 
> 
> BTW, does IPv6 MLD has this issue?

I will take a look at IPv6 MLD later. maybe tonight.
Thank you.

Zhengchao Shao
> 
> Thanks
> Hangbin
>
shaozhengchao Nov. 21, 2023, 2:36 p.m. UTC | #3
On 2023/11/21 10:56, Hangbin Liu wrote:
> Hi Zhengchao,
> On Tue, Nov 21, 2023 at 10:05:58AM +0800, Zhengchao Shao wrote:
>> ---
>>   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;
> 
> I haven't checked this part for a long time. What's the difference of in_dev->dev
> and im->interface->dev? I though they are the same, no?
> 
> If they are the same, should we stop processing the query earlier? e.g.
> 
> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index 76c3ea75b8dd..f4e1d229c9aa 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -1082,6 +1082,9 @@ int igmp_rcv(struct sk_buff *skb)
>                          goto drop;
>          }
> 
> +       if (!netif_running(dev))
> +               goto drop;
> +
>          in_dev = __in_dev_get_rcu(dev);
>          if (!in_dev)
>                  goto drop;
> 
> 
> BTW, does IPv6 MLD has this issue?
I also think mld has the same issue.

Thread A 				Thread B
icmpv6_rcv				br_dev_stop
   igmp6_event_query			  br_multicast_leave_snoopers
     start mc_query_work		            ipv6_dev_mc_dec
       Thread C				      __ipv6_dev_mc_dec
	mld_query_work				mutex_lock
	  ...				        igmp6_group_dropped//r=1
	  ...					mutex_unlock
	  ...					ma_put
	  ...					  refcount_dec_...//r=0
	  mutex_lock                  		
	  __mld_query_work
             igmp6_group_queried
	      refcount_inc(&ma->mca_refcnt) //r increased from 0
	  mutex_lock
Check whether the value of mcs_uses is 0 will solve the issue.

also I think checking whether the device is still running in IGMP does
not solve the uaf issue, but reduces the probability of the issue. I
will try to use a lock to solve the IGMP refcnt uaf issue.

> 
> Thanks
> Hangbin
>
Eric Dumazet Nov. 21, 2023, 5:37 p.m. UTC | #4
On Tue, Nov 21, 2023 at 2:53 AM Zhengchao Shao <shaozhengchao@huawei.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@huawei.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
>
shaozhengchao Nov. 22, 2023, 2:43 a.m. UTC | #5
On 2023/11/22 1:37, Eric Dumazet wrote:
> On Tue, Nov 21, 2023 at 2:53 AM Zhengchao Shao <shaozhengchao@huawei.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
> 
That's very usefull for me, Thank you. I will use it.

When locating the issue, I print the reference counting call stack of
the mc list when the issue occurs, like dev_tracker. As shown in the
following figure:
  node = ffff899f0113eb40
  stack count is 1, op : hold
[<0000000083ff3eef>] igmp_start_timer+0x4e/0xa0
[<00000000d52900df>] igmp_mod_timer+0x99/0xa8
[<0000000007b0df49>] igmp_heard_query.cold+0x3c/0x46
[<00000000a8401267>] igmp_rcv+0x142/0x2b0
[<00000000004cd82b>] ip_protocol_deliver_rcu+0x188/0x1c0
[<000000007133c934>] ip_local_deliver_finish+0x44/0x60
[<000000006dbfe577>] __netif_receive_skb_one_core+0x8b/0xa0
[<00000000c462a041>] netif_receive_skb_internal+0x40/0xd0
[<00000000963a8181>] netif_receive_skb+0x17/0x90
[<000000006b194425>] br_handle_frame_finish+0x17e/0x450 [bridge]
[<00000000a2ed8c7a>] nf_hook_bridge_pre+0x111/0x130 [bridge]
[<00000000154b9e87>] __netif_receive_skb_core+0x1a6/0xf20
[<000000001278b781>] __netif_receive_skb_list_core+0x13f/0x2e0
[<000000002d543b87>] __netif_receive_skb_list+0xfd/0x190
[<00000000de5ceec5>] netif_receive_skb_list_internal+0xfc/0x1e0
[<00000000a02df917>] gro_normal_one+0x77/0xa0
stack count is 1, op : put
[<000000005042a35a>] ip_ma_put+0x16/0xb0
[<0000000085f34370>] br_ip4_multicast_leave_snoopers.isra.0+0x47/0xa0 
[bridge]
[<00000000870fe473>] br_multicast_leave_snoopers+0x26/0x70 [bridge]
[<00000000043d0a0c>] br_dev_stop+0x4c/0x50 [bridge]
[<000000004c2d80b1>] __dev_close_many+0x99/0x110
[<00000000cdba8c2a>] __dev_change_flags+0x10d/0x250
[<000000004ee64457>] dev_change_flags+0x21/0x60
[<000000007aa8f47d>] devinet_ioctl+0x5c5/0x710
[<0000000060b50685>] inet_ioctl+0x190/0x1d0
[<00000000a89e60f7>] sock_do_ioctl+0x38/0x140
[<00000000b9071265>] sock_ioctl+0x195/0x370
[<00000000c24ede15>] __se_sys_ioctl+0x85/0xc0
[<00000000cb9d6bde>] do_syscall_64+0x33/0x40
[<00000000b8341b80>] entry_SYSCALL_64_after_hwframe+0x62/0xc7
stack count is 1, op : _put
[<00000000f49c2712>] igmp_stop_timer+0x4f/0x80
[<0000000094bd7946>] __igmp_group_dropped+0x79/0x1b0
[<000000003f7c8697>] __ip_mc_dec_group+0xc9/0xf0
  [<0000000085f34370>] br_ip4_multicast_leave_snoopers.isra.0+0x47/0xa0 
[bridge]
[<00000000870fe473>] br_multicast_leave_snoopers+0x26/0x70 [bridge]
[<00000000043d0a0c>] br_dev_stop+0x4c/0x50 [bridge]
[<000000004c2d80b1>] __dev_close_many+0x99/0x110
[<00000000cdba8c2a>] __dev_change_flags+0x10d/0x250
[<000000004ee64457>] dev_change_flags+0x21/0x60
[<000000007aa8f47d>] devinet_ioctl+0x5c5/0x710
[<0000000060b50685>] inet_ioctl+0x190/0x1d0
[<00000000a89e60f7>] sock_do_ioctl+0x38/0x140
[<00000000b9071265>] sock_ioctl+0x195/0x370
[<00000000c24ede15>] __se_sys_ioctl+0x85/0xc0
[<00000000cb9d6bde>] do_syscall_64+0x33/0x40
[<00000000b8341b80>] entry_SYSCALL_64_after_hwframe+0x62/0xc7
stack count is 1, op : hold
[<0000000083ff3eef>] igmp_start_timer+0x4e/0xa0
[<0000000062a4d9aa>] igmp_group_added+0x17b/0x1e0
[<000000004d79d41c>] ____ip_mc_inc_group+0x188/0x260
[<00000000ec98c0c0>] br_ip4_multicast_join_snoopers.isra.0+0x47/0x90 
[bridge]
[<0000000011f715b6>] br_multicast_join_snoopers+0x26/0x70 [bridge]
[<0000000062443b20>] br_dev_open+0x51/0x60 [bridge]
[<000000005333d1a7>] __dev_open+0xee/0x1a0
[<000000007024c19b>] __dev_change_flags+0x1de/0x250
[<000000004ee64457>] dev_change_flags+0x21/0x60
[<000000007aa8f47d>] devinet_ioctl+0x5c5/0x710
[<0000000060b50685>] inet_ioctl+0x190/0x1d0
[<00000000a89e60f7>] sock_do_ioctl+0x38/0x140
[<00000000b9071265>] sock_ioctl+0x195/0x370
[<00000000c24ede15>] __se_sys_ioctl+0x85/0xc0
[<00000000cb9d6bde>] do_syscall_64+0x33/0x40
[<00000000b8341b80>] entry_SYSCALL_64_after_hwframe+0x62/0xc7

Therefore, the process analysis of the issue is accurate.
>>
>> 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@huawei.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.
> 
I think it could solves the issue.

ThreadA			Thread B
			igmp_heard_query
__ip_mc_dec_group       ...
   __igmp_group_dropped  ...
     igmp_stop_timer     ...//r = 1
     ...                   rcu_read_lock
     ...                   igmp_mod_timer
     ...                     igmp_start_timer  //if timer is started, r=2
			
     ...                   rcu_read_unlock
   ip_ma_put //r=1
			
		     Thread C
		     igmp_timer_expire //timer function will be called
		       ip_ma_put //r=0,free im

Thanks

Zhengchao Shao
>>                  if (group && group != im->multiaddr)
>>                          continue;
>>                  if (im->multiaddr == IGMP_ALL_HOSTS)
>> --
>> 2.34.1
>>
>
diff mbox series

Patch

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;
 		if (group && group != im->multiaddr)
 			continue;
 		if (im->multiaddr == IGMP_ALL_HOSTS)