netdev - Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc

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: <CANn89iLUxP_YGLD1mrCmAr9qSg7wPWDjWPhJHNa_X4QVyNWqBQ@mail.gmail.com>
Date: Thu, 18 Jan 2024 09:59:54 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
Cc: "David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Taehee Yoo <ap420073@...il.com>, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+a9400cabb1d784e49abf@...kaller.appspotmail.com
Subject: Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

On Wed, Jan 17, 2024 at 6:21 PM Nikita Zhandarovich
<n.zhandarovich@...tech.ru> wrote:
>
> idev->mc_ifc_count can be written over without proper locking.
>
> Originally found by syzbot [1], fix this issue by encapsulating calls
> to mld_ifc_stop_work() (and mld_gq_stop_work() for good measure) with
> mutex_lock() and mutex_unlock() accordingly as these functions
> should only be called with mc_lock per their declarations.
>
> [1]
> BUG: KCSAN: data-race in ipv6_mc_down / mld_ifc_work
>
> Fixes: 2d9a93b4902b ("mld: convert from timer to delayed work")
> Reported-by: syzbot+a9400cabb1d784e49abf@...kaller.appspotmail.com
> Link: https://lore.kernel.org/all/000000000000994e09060ebcdffb@google.com/
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
> ---
>  net/ipv6/mcast.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index b75d3c9d41bb..bc6e0a0bad3c 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -2722,8 +2722,12 @@ void ipv6_mc_down(struct inet6_dev *idev)
>         synchronize_net();
>         mld_query_stop_work(idev);
>         mld_report_stop_work(idev);
> +
> +       mutex_lock(&idev->mc_lock);
>         mld_ifc_stop_work(idev);
>         mld_gq_stop_work(idev);
> +       mutex_unlock(&idev->mc_lock);
> +
>         mld_dad_stop_work(idev);
>  }
>

Thanks for the fix.

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

I would also add some lockdep_assert_held() to make sure assumptions are met.
Trading a comment for a runtime check is better.

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index b75d3c9d41bb5005af2d4e10fab58f157e9ea4fa..b256362d3b5d5111f649ebfee4f1557d8c063d92
100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1047,36 +1047,36 @@ bool ipv6_chk_mcast_addr(struct net_device
*dev, const struct in6_addr *group,
        return rv;
 }

-/* called with mc_lock */
 static void mld_gq_start_work(struct inet6_dev *idev)
 {
        unsigned long tv = get_random_u32_below(idev->mc_maxdelay);

+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_gq_running = 1;
        if (!mod_delayed_work(mld_wq, &idev->mc_gq_work, tv + 2))
                in6_dev_hold(idev);
 }

-/* called with mc_lock */
 static void mld_gq_stop_work(struct inet6_dev *idev)
 {
+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_gq_running = 0;
        if (cancel_delayed_work(&idev->mc_gq_work))
                __in6_dev_put(idev);
 }

-/* called with mc_lock */
 static void mld_ifc_start_work(struct inet6_dev *idev, unsigned long delay)
 {
        unsigned long tv = get_random_u32_below(delay);

+       lockdep_assert_held(&idev->mc_lock);
        if (!mod_delayed_work(mld_wq, &idev->mc_ifc_work, tv + 2))
                in6_dev_hold(idev);
 }

-/* called with mc_lock */
 static void mld_ifc_stop_work(struct inet6_dev *idev)
 {
+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_ifc_count = 0;
        if (cancel_delayed_work(&idev->mc_ifc_work))
                __in6_dev_put(idev);