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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ