| 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
| ||
|
Message-ID: <8457ff3d-f033-8d06-42ca-d95f77ec8145@gmail.com>
Date: Wed, 14 Jun 2023 11:23:44 +0200
From: Johan Jonker <jbx6244@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: richard@....at, vigneshr@...com, heiko@...ech.de,
linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, yifeng.zhao@...k-chips.com
Subject: Re: [PATCH v2 3/5] mtd: nand: raw: rockchip-nand-controller: fix
oobfree offset and description
On 6/12/23 19:26, Miquel Raynal wrote:
> Hi Johan,
>
> jbx6244@...il.com wrote on Mon, 12 Jun 2023 17:03:18 +0200:
>
>> The MTD framework reserves 1 or 2 bytes for the bad block marker
>> depending on the bus size. The rockchip-nand-controller driver
>> currently only supports a 8 bit bus, but reserves standard 2 bytes
>> for the BBM.
>
> We always reserve 2 bytes, no?
Not always used, but for consistency/simplicity the author assumes/reserves 2 bytes.
>
>> The first free OOB byte is therefore OOB2 at offset 2.
>> Page address(PA) bytes are moved to the last 4 positions before
>> ECC. Update the description for Linux.
>
> The description should just be:
>
> Move Page Address (PA) bytes to the last 4 positions before ECC.
Space is already reserved, but overwritten.
>
> And then you should justify why this is needed. Also, this would break
> all existing jffs2 users, right?
Hi Miquel,
>From your comments it seems that the chip->oob_poi buffer layout is still not clear to you.
Hope that this text below helps.
If existing jffs2 users of free OOB are writing they are corrupting our PA data in RAW mode.
So that must be fixed. Please advise how we split pre and post change users.
(With a Module parameter like skipbbt renamed to "user_mode" = 0 offset 6, "user_mode" = 1 offset 2)
Copying PA data in both RAW and HW mode has already reserved space in the layout.
Let me know if I can help to get forward here.
Johan
===
Given:
Rockchip rk3066 MK808 with NAND:
nand: Hynix H27UCG8T2ATR-BC 64G 3.3V 8-bit
nand: 8192 MiB, MLC, erase size: 2048 KiB, page size: 8192, OOB size: 640
===
Calulations:
#define NFC_SYS_DATA_SIZE (4) /* 4 bytes sys data in oob pre 1024 data.*/
So per step only 4 bytes of OOB can be read.
===
The NFC can read/write in 1024 data bytes per step.
To read/write a full page it needs 8 steps.
chip->ecc.size = 1024;
chip->ecc.steps = mtd->writesize / chip->ecc.size;
= 8192 / 1024
= 8 steps
===
The total size of usefull OOB before ECC:
rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
= 4 * 8
= 32
===
Wrong free OOB offset starts at OOB6:
oob_region->offset = NFC_SYS_DATA_SIZE + 2;
= 4 + 2
= 6
With a free OOB offset of 6 and a length of 26 ==> 6 + 26 = 32 we corrupt the PA address starting at offset 28.
New offset OOB2:
oob_region->offset = 2;
The full range of free runs from OOB2 till/including OOB27.
===
The last 4 bytes of metadata are reserved for this Page Address(PA) for the bootrom.
Currently only in use in RAW mode.
The current PA calculation needed to write boot blocks for all Rockchip SoCs is however useless.
The pattern of where the next page is written depends on the chip ID.
As the MTD framework doesn't pass this chip ID in it's data structures,
we must calculate that in userspace.
Therefore both RAW and HW mode must pass the PA bytes.
===
The NFC hardware is capable for a 16 bit bus, but not implemented yet.
Reserved are standard 2 bits for the BBM for a consistantency by the original author.
===
chip->oob_poi buffer layout for 8 steps:
BBM0 BBM1 OOB2 OOB3 | OOB4 OOB5 OOB6 OOB7
OOB8 OOB9 OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15
OOB16 OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23
OOB24 OOB25 OOB26 OOB27 | PA0 PA1 PA2 PA3
ECC0 ECC1 ECC2 ECC3 | ... ... ... ...
===
rk_nfc_ooblayout_free:
oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
= 32 - 4 - 2
= 26
oob_region->offset = 2;
Free OOB should start at OOB2 to not overwrite PA data.
===
rk_nfc_ooblayout_ecc:
oob_region->length = mtd->oobsize - rknand->metadata_size;
= 640 - 32
= 608
oob_region->offset = rknand->metadata_size;
= 32
ECC data starts at offset 32.
===
>
>>
>> Signed-off-by: Johan Jonker <jbx6244@...il.com>
>> ---
>> drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> index 31d8c7a87..fcda4c760 100644
>> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> @@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
>> * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3
>> *
>> * The rk_nfc_ooblayout_free() function already has reserved
>> - * these 4 bytes with:
>> + * these 4 bytes together with 2 bytes for BBM
>> + * by reducing it's length:
>> *
>> - * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> + * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>> */
>> if (!i)
>> memcpy(rk_nfc_oob_ptr(chip, i),
>> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
>> if (section)
>> return -ERANGE;
>>
>> - /*
>> - * The beginning of the OOB area stores the reserved data for the NFC,
>> - * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
>> - */
>> oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>> - oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> + oob_region->offset = 2;
>>
>> return 0;
>> }
>> --
>> 2.30.2
>>
>
>
> Thanks,
> Miquèl
Powered by blists - more mailing lists