linux-kernel - Re: [PATCH v6 05/10] mtd: get the ECC info from the Extended Parameter Page

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 May 2013 23:06:39 +0530
From:	Vikram Narayanan <vikram186@...il.com>
To:	Huang Shijie <b32955@...escale.com>
CC:	dwmw2@...radead.org, dedekind1@...il.com,
	computersforpeace@...il.com, linux-mtd@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 05/10] mtd: get the ECC info from the Extended Parameter
 Page

Hello Huang,

On 5/17/2013 8:47 AM, Huang Shijie wrote:
> Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page
> to store the ECC info.
>
<snip>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b63b731..0b1162a 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2835,6 +2835,71 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
>   	return crc;
>   }
>
> +/* Parse the Extended Parameter Page. */
> +static int nand_flash_detect_ext_param_page(struct mtd_info *mtd,
> +		struct nand_chip *chip, struct nand_onfi_params *p)
> +{
<snip>
> +
> +	/* Read out the Extended Parameter Page. */
> +	chip->read_buf(mtd, (uint8_t *)ep, len);
> +	if ((onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2)
> +		!= le16_to_cpu(ep->crc)) || strncmp(ep->sig, "EPPS", 4)) {

 From section 5.7.2.2.
///Byte 2-5: Extended parameter page signature
This field contains the extended parameter page signature. When two or 
more bytes of the signature are valid, then it denotes that a valid copy 
of the extended parameter page is present///

But here you are doing a strict check. What if the first two bytes are 
valid? Or did I misunderstood something?
If not, I'd prefer to take the strncmp to a separate 'if' so, that you 
can do the comparison in the way specified in the ONFI spec.

> +		pr_debug("fail in the CRC.\n");

Also, this is the error for Signature failure as well. Please move the 
signature comparison to a new if to give better error messages.

> +		ret = -EINVAL;
> +		goto ext_out;
> +	}

Regards,
Vikram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/