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
| ||
|
Date: Tue, 12 May 2020 09:24:32 +0200 From: Álvaro Fernández Rojas <noltari@...il.com> To: Miquel Raynal <miquel.raynal@...tlin.com> Cc: computersforpeace@...il.com, kdasu.kdev@...il.com, richard@....at, vigneshr@...com, sumit.semwal@...aro.org, linux-mtd@...ts.infradead.org, bcm-kernel-feedback-list@...adcom.com, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org Subject: Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages Hi Miquèl > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@...tlin.com> escribió: > > Hi Álvaro, > > Álvaro Fernández Rojas <noltari@...il.com> wrote on Tue, 12 May 2020 > 08:51:11 +0200: > >> The current code checks that the whole OOB area is erased. >> This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will >> fail due to the usable OOB bytes not being 0xff. >> Correct this by only checking that data and ECC bytes aren't 0xff. >> >> Fixes: 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips") >> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com> >> --- >> v3: Fix commit log and merge nand_check_erased_ecc_chunk calls. >> v2: Add Fixes tag >> >> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> index e4e3ceeac38f..80fe01f03516 100644 >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> @@ -2018,8 +2018,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, >> static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, >> struct nand_chip *chip, void *buf, u64 addr) >> { >> + struct mtd_oob_region oobecc; >> int i, sas; >> - void *oob = chip->oob_poi; >> + void *oob; >> int bitflips = 0; >> int page = addr >> chip->page_shift; >> int ret; >> @@ -2035,11 +2036,19 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, >> if (ret) >> return ret; >> >> - for (i = 0; i < chip->ecc.steps; i++, oob += sas) { >> + for (i = 0; i < chip->ecc.steps; i++) { >> ecc_chunk = buf + chip->ecc.size * i; >> - ret = nand_check_erased_ecc_chunk(ecc_chunk, >> - chip->ecc.size, >> - oob, sas, NULL, 0, >> + >> + if (mtd->ooblayout->ecc(mtd, i, &oobecc)) { > > Please use the mtdcore.c's helpers > (mtd_ooblayout_set/get_data/free/ecc/bytes). > > Also, what are you trying to discriminate with the return code of the > function? Shouldn't this function "always" work? Just making sure it doesn’t return an ERANGE in case chip->ecc.size doesn’t match the sections from mtd->ooblayout->ecc, which shouldn’t happen, so I think we can remove that... > >> + oob = NULL; >> + oobecc.length = 0; >> + } else { >> + oob = chip->oob_poi + oobecc.offset; >> + } >> + >> + ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size, >> + oob, oobecc.length, >> + NULL, 0, >> chip->ecc.strength); > > As I told you, this helper takes "maid data" then "spare area" then > "ecc bytes". The names are pretty important here as you want to avoid > checking the spare OOB bytes on purpose, so maybe you could have more > meaningful names and call "ecc" instead of "oob" the ecc region? Actually I thought you meant the commit log, not the code itself... > >> if (ret < 0) >> return ret; > > > Thanks, > Miquèl Regards, Álvaro.
Powered by blists - more mailing lists