netdev - Re: [PATCH] forcedeth: mac address fix

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: <20080401000502.GB7423@atjola.homenet>
Date:	Tue, 1 Apr 2008 02:05:02 +0200
From:	Björn Steinbrink <B.Steinbrink@....de>
To:	Ayaz Abdulla <aabdulla@...dia.com>
Cc:	Jeff Garzik <jgarzik@...ox.com>,
	Manfred Spraul <manfred@...orfullife.com>,
	Andrew Morton <akpm@...l.org>, nedev <netdev@...r.kernel.org>
Subject: Re: [PATCH] forcedeth: mac address fix

On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
> This critical patch fixes a mac address issue recently introduced. If  

Does "recently" mean my commit 2e3884b5b16795c03a7bf295797c1b2402885b88?
If so, I like to be told directly when I break stuff ;-)

> the device's mac address was in correct order and the flag  
> NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag would  
> get cleared. During next load, the mac address would get reversed  
> because the flag is missing.

Hm, but nv_remove also writes back the reversed mac address. I don't see
how a plain remove/probe cycle would mess things up.

> As it has been indicated previously, the flag is cleared across a low  
> power transition. Therefore, the driver should set the mac address back  
> into the reversed order when clearing the flag.

That's what nv_remove is supposed to do. Is there a case where nv_remove
is not called?

> Also, the driver should set back the flag after a low power transition  
> to protect against kexec command calling nv_probe a second time.

Sounds like suspend stopped calling nv_remove? That would make sense
then. I never checked whether suspend ever actually did call nv_remove
(I think), but as my patch worked, it basically must have done so, at
least in the past, right?

Thanks,
Björn

>
> Signed-off-by: Ayaz Abdulla <aabdulla@...dia.com>
>

> --- old/drivers/net/forcedeth.c	2008-03-31 15:25:05.000000000 -0700
> +++ new/drivers/net/forcedeth.c	2008-03-31 15:41:51.000000000 -0700
> @@ -5317,8 +5317,7 @@
>  
>  	/* check the workaround bit for correct mac address order */
>  	txreg = readl(base + NvRegTransmitPoll);
> -	if ((txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) ||
> -	    (id->driver_data & DEV_HAS_CORRECT_MACADDR)) {
> +	if (id->driver_data & DEV_HAS_CORRECT_MACADDR) {
>  		/* mac address is already in correct order */
>  		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
>  		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
> @@ -5326,6 +5325,22 @@
>  		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
>  		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
>  		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
> +	} else if (txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) {
> +		/* mac address is already in correct order */
> +		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
> +		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
> +		dev->dev_addr[2] = (np->orig_mac[0] >> 16) & 0xff;
> +		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
> +		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
> +		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
> +		/*
> +		 * Set orig mac address back to the reversed version.
> +		 * This flag will be cleared during low power transition.
> +		 * Therefore, we should always put back the reversed address.
> +		 */
> +		np->orig_mac[0] = (dev->dev_addr[5] << 0) + (dev->dev_addr[4] << 8) +
> +			(dev->dev_addr[3] << 16) + (dev->dev_addr[2] << 24);
> +		np->orig_mac[1] = (dev->dev_addr[1] << 0) + (dev->dev_addr[0] << 8);
>  	} else {
>  		/* need to reverse mac address to correct order */
>  		dev->dev_addr[0] = (np->orig_mac[1] >>  8) & 0xff;
> @@ -5596,7 +5611,9 @@
>  static int nv_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	u8 __iomem *base = get_hwbase(dev);
>  	int rc = 0;
> +	u32 txreg;
>  
>  	if (!netif_running(dev))
>  		goto out;
> @@ -5607,6 +5624,11 @@
>  	pci_restore_state(pdev);
>  	pci_enable_wake(pdev, PCI_D0, 0);
>  
> +	/* restore mac address reverse flag */
> +	txreg = readl(base + NvRegTransmitPoll);
> +	txreg |= NVREG_TRANSMITPOLL_MAC_ADDR_REV;
> +	writel(txreg, base + NvRegTransmitPoll);
> +
>  	rc = nv_open(dev);
>  out:
>  	return rc;

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html