netdev - Re: [PATCH net-next 1/3] net: bridge: fix ioctl locking

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: <53339ad0-1028-2660-3241-58ee70d8247a@nvidia.com>
Date:   Thu, 5 Aug 2021 12:29:34 +0300
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Nikolay Aleksandrov <razor@...ckwall.org>, netdev@...r.kernel.org
Cc:     roopa@...dia.com, arnd@...db.de, bridge@...ts.linux-foundation.org,
        syzbot+34fe5894623c4ab1b379@...kaller.appspotmail.com,
        Hillf Danton <hdanton@...a.com>
Subject: Re: [PATCH net-next 1/3] net: bridge: fix ioctl locking

On 05/08/2021 11:29, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@...dia.com>
> 
> Before commit ad2f99aedf8f ("net: bridge: move bridge ioctls out of
> .ndo_do_ioctl") the bridge ioctl calls were divided in two parts:
> one was deviceless called by sock_ioctl and didn't expect rtnl to be held,
> the other was with a device called by dev_ifsioc() and expected rtnl to be
> held. After the commit above they were united in a single ioctl stub, but
> it didn't take care of the locking expectations.
> For sock_ioctl now we acquire  (1) br_ioctl_mutex, (2) rtnl
> and for dev_ifsioc we acquire  (1) rtnl,           (2) br_ioctl_mutex
> 
> The fix is to get a refcnt on the netdev for dev_ifsioc calls and drop rtnl
> then to reacquire it in the bridge ioctl stub after br_ioctl_mutex has
> been acquired. That will avoid playing locking games and make the rules
> straight-forward: we always take br_ioctl_mutex first, and then rtnl.
> 
> Reported-by: syzbot+34fe5894623c4ab1b379@...kaller.appspotmail.com
> Fixes: ad2f99aedf8f ("net: bridge: move bridge ioctls out of .ndo_do_ioctl")
> Signed-off-by: Nikolay Aleksandrov <nikolay@...dia.com>
> ---
>  net/bridge/br_if.c    |  4 +---
>  net/bridge/br_ioctl.c | 37 ++++++++++++++++++++++++-------------
>  net/core/dev_ioctl.c  |  7 ++++++-
>  3 files changed, 31 insertions(+), 17 deletions(-)
> 
[snip]

I fixed the bridge side of things, but the unlock/lock suggestion was made first by Hillf.
I forgot to add:

Suggested-by: Hillf Danton <hdanton@...a.com>

+CC Hillf

Hillf, since the rtnl unlock/lock suggestion was yours feel free to add
your signed-off-by

Thanks,
 Nik