linux-kernel - Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking

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]

Date:   Wed, 22 Mar 2017 21:56:23 +0100
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     linux-mtd@...ts.infradead.org, laurent.monat@...uantique.com,
        thorsten.christiansson@...uantique.com,
        Enrico Jorns <ejo@...gutronix.de>,
        Jason Roberts <jason.e.roberts@...el.com>,
        Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Brian Norris <computersforpeace@...il.com>,
        Graham Moore <grmoore@...nsource.altera.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Chuanxiao Dong <chuanxiao.dong@...el.com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        linux-kernel@...r.kernel.org, Richard Weinberger <richard@....at>,
        Cyrille Pitchen <cyrille.pitchen@...el.com>
Subject: Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking

On Wed, 22 Mar 2017 23:07:17 +0900
Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
>  		dev_err(denali->dev,
> @@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	if (check_erased_page) {
>  		read_oob_data(mtd, chip->oob_poi, denali->page);
>  
> -		/* check ECC failures that may have occurred on erased pages */
> -		if (check_erased_page) {
> -			if (!is_erased(buf, mtd->writesize))
> -				mtd->ecc_stats.failed++;
> -			if (!is_erased(buf, mtd->oobsize))
> -				mtd->ecc_stats.failed++;
> +		stat = nand_check_erased_ecc_chunk(
> +					buf, mtd->writesize,
> +					chip->oob_poi, mtd->oobsize,
> +					NULL, 0,
> +					chip->ecc.strength * chip->ecc.steps);

That's not how it's supposed to be done. Each chunk should be checked
independently. Here is a simple example explaining why this is
important:

Let's consider the following setup:
- 4k pages
- 16bits/1024bytes ECC

With your approach, you turn this into:
- 4k pages
- 64bits/4096bytes ECC

Now suppose you have 32 bitflips in the first 1024 bytes. The real ECC
config is expected to report uncorrectable errors, but your approach
will just report that 32 bits have been fixed, which is wrong.