| Message ID | 20240408083605.55238-2-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/nand: Fix out-of-bound access in NAND block buffer | expand |
On 4/7/24 22:36, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org> > --- > hw/block/nand.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Am 08.04.2024 um 10:36 hat Philippe Mathieu-Daudé geschrieben: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/block/nand.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/hw/block/nand.c b/hw/block/nand.c > index d1435f2207..6fa9038bb5 100644 > --- a/hw/block/nand.c > +++ b/hw/block/nand.c > @@ -243,9 +243,25 @@ static inline void nand_pushio_byte(NANDFlashState *s, uint8_t value) > } > } > > +/* > + * nand_load_block: Load block containing (s->addr + @offset). > + * Returns length of data available at @offset in this block. > + */ > +static int nand_load_block(NANDFlashState *s, int offset) > +{ > + int iolen; > + > + s->blk_load(s, s->addr, offset); Wouldn't it make more sense for @offset to be unsigned, like in nand_command() before this patch? I think the values are guaranteed to be small enough to fit in either signed or unsigned, but we never check for < 0 (maybe that should be done in your patch to s->blk_load() anyway). > + iolen = (1 << s->page_shift) - offset; This is not new, but I'm confused. Can this legitimately be negative? offset seems to be limited to (1 << s->addr_shift) + s->offset in practice, but addr_shift > page_shift for NAND_PAGE_SIZE == 2048. After patch 3, offset is implicitly limited to NAND_PAGE_SIZE + OOB_SIZE because we return early if s->blk_load() fails. I wonder if it wouldn't be better to catch this in the callers already and only assert in s->blk_load(). Anyway, after patch 3 iolen can only temporarily become negative here... > + if (s->gnd) { > + iolen += 1 << s->oob_shift; ...before it becomes non-negative again here. > + } > + return iolen; > +} So none of this makes the code technically incorrect after applying the full series, but let someone modify it who doesn't understand these intricacies and I think chances are that it will fall apart. Kevin
diff --git a/hw/block/nand.c b/hw/block/nand.c index d1435f2207..6fa9038bb5 100644 --- a/hw/block/nand.c +++ b/hw/block/nand.c @@ -243,9 +243,25 @@ static inline void nand_pushio_byte(NANDFlashState *s, uint8_t value) } } +/* + * nand_load_block: Load block containing (s->addr + @offset). + * Returns length of data available at @offset in this block. + */ +static int nand_load_block(NANDFlashState *s, int offset) +{ + int iolen; + + s->blk_load(s, s->addr, offset); + + iolen = (1 << s->page_shift) - offset; + if (s->gnd) { + iolen += 1 << s->oob_shift; + } + return iolen; +} + static void nand_command(NANDFlashState *s) { - unsigned int offset; switch (s->cmd) { case NAND_CMD_READ0: s->iolen = 0; @@ -271,12 +287,7 @@ static void nand_command(NANDFlashState *s) case NAND_CMD_NOSERIALREAD2: if (!(nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP)) break; - offset = s->addr & ((1 << s->addr_shift) - 1); - s->blk_load(s, s->addr, offset); - if (s->gnd) - s->iolen = (1 << s->page_shift) - offset; - else - s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset; + s->iolen = nand_load_block(s, s->addr & ((1 << s->addr_shift) - 1)); break; case NAND_CMD_RESET: @@ -597,12 +608,7 @@ uint32_t nand_getio(DeviceState *dev) if (!s->iolen && s->cmd == NAND_CMD_READ0) { offset = (int) (s->addr & ((1 << s->addr_shift) - 1)) + s->offset; s->offset = 0; - - s->blk_load(s, s->addr, offset); - if (s->gnd) - s->iolen = (1 << s->page_shift) - offset; - else - s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset; + s->iolen = nand_load_block(s, offset); } if (s->ce || s->iolen <= 0) {
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/block/nand.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)