linux-kernel - Re: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write

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

Date:   Fri, 1 Mar 2019 17:07:15 +0100
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     "Tokunori Ikegami" <ikegami_to@...oo.co.jp>
Cc:     "'liujian \(CE\)'" <liujian56@...wei.com>,
        "'Tokunori Ikegami'" <ikegami.t@...il.com>,
        <keescook@...omium.org>, <bbrezillon@...nel.org>,
        <ikegami@...ied-telesis.co.jp>, <richard@....at>,
        <linux-kernel@...r.kernel.org>, <marek.vasut@...il.com>,
        <linux-mtd@...ts.infradead.org>, <computersforpeace@...il.com>,
        <dwmw2@...radead.org>, <vigneshr@...com>
Subject: Re: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c
 do_write_buffer

Hi Ikegami,

On Fri, 1 Mar 2019 23:51:16 +0900
"Tokunori Ikegami" <ikegami_to@...oo.co.jp> wrote:

> > Except this version no longer does what Vignesh suggested. See how you
> > no longer test if chip_good() is true if time_after() returns true. So,
> > imagine the thread entering this function is preempted just after the
> > first chip_good() test, and resumed a few ms later. time_after() will
> > return true, but chip_good() might also return true, and you ignore it.  
> 
> I think that the following 3 versions will be worked for time_after() as a same result and follow the Vignesh-san suggestion.

Let me show you how they are different:

> 
> 1. Original Vignesh-san suggestion
> 
> 	if (chip_good(map, adr, datum)) {
> 		xip_enable(map, chip, adr);
> 		goto op_done;
> 	}

--> thread preempted here
==> chip_good() test becomes valid here
--> thread resumed here, but timeout has expired

> 
> 	if (time_after(jiffies, timeo)) {

you enter this branch

> 		/* Test chip_good() if TRUE incorrectly again so write failure by time_after() can be avoided. */
> 		if (chip_good(map, adr, datum)) {

chip_good() returns true

> 			xip_enable(map, chip, adr);
> 			goto op_done;
> 		}
> 		break;
> 	}
> 
> 2. Liujian-san v3 patch
> 
> 	/* Test chip_good() if FALSE correctly so write failure by time_after() can be avoided. */

--> thread preempted here
==> chip_good() test becomes valid here
--> thread resumed here, but timeout has expired

> 	if (time_after(jiffies, timeo) && !chip_good(map, adr))

You do not enter this branch because the chip_good() test is done once
more in case of timeout.

> 		break;
> 
> 	if (chip_good(map, adr, datum)) {
> 		xip_enable(map, chip, adr);
> 		goto op_done;
> 	}
> 
> 3. My idea
> 
> 	/* Save current jiffies value before chip_good() to avoid write failure by time_after() without testing chip_good() again. */
> 	unsigned long now = jiffies;
> 
> 	if (chip_good(map, adr, datum)) {
> 		xip_enable(map, chip, adr);
> 		goto op_done;
> 	}
> 

--> thread preempted here
==> chip_good() test becomes valid here
--> thread resumed here, but timeout has expired

> 	if (time_after(now, timeo))

You do enter this branch, and erroneously report a failure.

> 		break;
> 

See now why your version is not correct?

Regards,

Boris