linux-kernel - Re: [PATCH 1/2] net: axienet: recheck condition after timeout in mdio

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: <20181030.112511.957438343014682098.davem@davemloft.net>
Date:   Tue, 30 Oct 2018 11:25:11 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     kurt@...utronix.de
Cc:     anirudh@...inx.com, John.Linn@...inx.com, michal.simek@...inx.com,
        radhey.shyam.pandey@...inx.com, andrew@...n.ch,
        yuehaibing@...wei.com, netdev@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] net: axienet: recheck condition after timeout in
 mdio_wait()

From: Kurt Kanzenbach <kurt@...utronix.de>
Date: Tue, 30 Oct 2018 10:31:38 +0100

> The function could report a false positive if it gets preempted between reading
> the XAE_MDIO_MCR_OFFSET register and checking for the timeout.  In such a case,
> the condition has to be rechecked to avoid false positives.
> 
> Therefore, check for expected condition even after the timeout occurred.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
 ...
>  		if (time_before_eq(end, jiffies)) {
> -			WARN_ON(1);
> -			return -ETIMEDOUT;
> +			val = axienet_ior(lp, XAE_MDIO_MCR_OFFSET);
> +			break;
>  		}
> +
>  		udelay(1);
>  	}
> -	return 0;
> +	if (val & XAE_MDIO_MCR_READY_MASK)
> +		return 0;
> +
> +	WARN_ON(1);
> +	return -ETIMEDOUT;

You are not fundamentally changing the situation at all.

The condtion could change right after your last read of
XAR_MDIO_MCR_OFFSET, which is the same thing that happens before your
modifications to this code.

It sounds more like the timeout is slightly too short, and that's the
real problem that causes whatever behavior you think you are fixing
here.

I'm not applying this.