netdev - Re: [PATCH net] mctp: Remove device type check at unregister

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: <7ee98dcab99a4fd23323f6cec803ef6c008118b0.camel@gmail.com>
Date:   Wed, 14 Dec 2022 08:42:04 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Matt Johnston <matt@...econstruct.com.au>, netdev@...r.kernel.org
Cc:     pabeni@...hat.com, kuba@...nel.org, edumazet@...gle.com,
        davem@...emloft.net, jk@...econstruct.com.au
Subject: Re: [PATCH net] mctp: Remove device type check at unregister

On Wed, 2022-12-14 at 14:10 +0800, Matt Johnston wrote:
> The check could be incorrectly triggered if a netdev
> changes its type after register. That is possible for a tun device
> using TUNSETLINK ioctl, resulting in mctp unregister failing
> and the netdev unregister waiting forever.
> 
> This was encountered by https://github.com/openthread/openthread/issues/8523
> 
> The check is not required, it was added in an attempt to track down mctp_ptr
> being set unexpectedly, which should not happen in normal operation.
> 
> Fixes: 7b1871af75f3 ("mctp: Warn if pointer is set for a wrong dev type")
> Signed-off-by: Matt Johnston <matt@...econstruct.com.au>
> ---
>  net/mctp/device.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index 99a3bda8852f..dec730b5fe7e 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -429,12 +429,6 @@ static void mctp_unregister(struct net_device *dev)
>  	struct mctp_dev *mdev;
>  
>  	mdev = mctp_dev_get_rtnl(dev);
> -	if (mdev && !mctp_known(dev)) {
> -		// Sanity check, should match what was set in mctp_register
> -		netdev_warn(dev, "%s: BUG mctp_ptr set for unknown type %d",
> -			    __func__, dev->type);
> -		return;
> -	}
>  	if (!mdev)
>  		return;
>  

It looks like this is incomplete if we are going to allow for these
type of changes. We might as well also remove the block in
mctp_register that was doing a similar check for devices that already
have the mctp_ptr set. Otherwise you will likely need to follow up on
this later.

Alternatively if you are wanting to be consistent you could just drop
the "return", in which case you will still get the warn in both cases
but it doesn't seem to affect behavior otherwise.

In addition you may want to look at registering a notifier to catch
NETDEV_PRE_TYPE_CHANGE and NETDEV_PRE_TYPE_CHANGE. It would enable you
to catch these types of things in the future if you are wanting to
allow changing types.