linux-kernel - Re: [PATCH net-next v7 3/7] net: macsec: revert the MAC address if mdo_upd

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] [day] [month] [year] [list]

Message-ID: <ZTKEdvpNl-gwGpk6@hog>
Date:   Fri, 20 Oct 2023 15:45:26 +0200
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, andrew@...n.ch, hkallweit1@...il.com,
        linux@...linux.org.uk, richardcochran@...il.com,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        sebastian.tobuschat@....nxp.com
Subject: Re: [PATCH net-next v7 3/7] net: macsec: revert the MAC address if
 mdo_upd_secy fails

2023-10-19, 15:02:05 +0300, Radu Pirea (NXP OSS) wrote:
>  	/* If h/w offloading is available, propagate to the device */
> -	if (macsec_is_offloaded(macsec)) {
> +	if (offloaded) {
>  		const struct macsec_ops *ops;
>  		struct macsec_context ctx;
>  
>  		ops = macsec_get_ops(macsec, &ctx);
> -		if (ops) {
> -			ctx.secy = &macsec->secy;
> -			macsec_offload(ops->mdo_upd_secy, &ctx);
> +		if (!ops) {
> +			err = -EINVAL;

For consistency with other places where macsec_get_ops fails, this
should probably be -EOPNOTSUPP.

I'm not opposed to this change, but I'm not sure how it's related to
the missing rollback issue. Can you explain that a bit?

> +			goto restore_old_addr;
>  		}
> +
> +		ctx.secy = &macsec->secy;
> +		err = macsec_offload(ops->mdo_upd_secy, &ctx);
> +		if (err)
> +			goto restore_old_addr;
>  	}
>  
>  	return 0;
> +
> +restore_old_addr:
> +	if (dev->flags & IFF_UP) {
> +		int err;

[This bit confused me quite a bit, seeing the declaration and the
"return err" out of this block]

> +		err = macsec_dev_uc_update(dev, old_addr);
> +		if (err)
> +			return err;

If we can avoid it, we should try to have a rollback path without any
possible failures, otherwise we'll leave things in a broken state (in
this case, telling the user that the MAC address change failed, but
leaving the new address set on the macsec device).

Paolo suggested to postpone the dev_uc_del until after the offload
code, so we'd end up with something like this [completely untested]:


	if (dev->flags & IFF_UP) {
		err = dev_uc_add(real_dev, addr->sa_data);
		if (err < 0)
			return err;
	}

	ether_addr_copy(old_addr, dev->dev_addr);
	eth_hw_addr_set(dev, addr->sa_data);

	/* If h/w offloading is available, propagate to the device */
	if (macsec_is_offloaded(macsec)) {
		// ...
	}

	if (dev->flags & IFF_UP)
		dev_uc_del(real_dev, old_addr);

	return 0;

restore_old_addr:
	if (dev->flags & IFF_UP)
		dev_uc_del(real_dev, addr->sa_data);

	eth_hw_addr_set(dev, old_addr);

	return err;


Install both UC addresses, then remove the one we don't need.


-- 
Sabrina