mbox series

[PATCH-for-9.0,v2,0/3] hw/block/nand: Fix out-of-bound access in NAND block buffer

Message ID 20240409135944.24997-1-philmd@linaro.org
Headers show
Series hw/block/nand: Fix out-of-bound access in NAND block buffer | expand

Message

Philippe Mathieu-Daudé April 9, 2024, 1:59 p.m. UTC 
Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446

Since v1:
- Addressed Kevin trivial suggestions (unsigned offset)

Philippe Mathieu-Daudé (3):
  hw/block/nand: Factor nand_load_iolen() method out
  hw/block/nand: Have blk_load() take unsigned offset and return boolean
  hw/block/nand: Fix out-of-bound access in NAND block buffer

 hw/block/nand.c | 55 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 17 deletions(-)

Comments

Philippe Mathieu-Daudé April 9, 2024, 2:04 p.m. UTC | #1 
On 9/4/24 15:59, Philippe Mathieu-Daudé wrote:
> Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446
> 
> Since v1:
> - Addressed Kevin trivial suggestions (unsigned offset)

$ git backport-diff
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, 
respectively

001/       3:[0009] [FC] 'hw/block/nand: Factor nand_load_iolen() method 
out'
002/       3:[0004] [FC] 'hw/block/nand: Have blk_load() return boolean 
indicating success'
003/       3:[----] [-C] 'hw/block/nand: Fix out-of-bound access in NAND 
block buffer'

$ git diff
diff --git a/hw/block/nand.c b/hw/block/nand.c
index d90dc965a1..e2433c25bd 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -88,7 +88,7 @@ struct NANDFlashState {
       * Returns %true when block containing (@addr + @offset) is
       * successfully loaded, otherwise %false.
       */
-    bool (*blk_load)(NANDFlashState *s, uint64_t addr, int offset);
+    bool (*blk_load)(NANDFlashState *s, uint64_t addr, unsigned offset);

      uint32_t ioaddr_vmstate;
  };
@@ -251,18 +251,21 @@ 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)
+static unsigned nand_load_block(NANDFlashState *s, unsigned offset)
  {
-    int iolen;
+    unsigned iolen;

      if (!s->blk_load(s, s->addr, offset)) {
          return 0;
      }

-    iolen = (1 << s->page_shift) - offset;
+    iolen = (1 << s->page_shift);
      if (s->gnd) {
          iolen += 1 << s->oob_shift;
      }
+    assert(offset <= iolen);
+    iolen -= offset;
+
      return iolen;
  }

@@ -776,7 +779,7 @@ static void glue(nand_blk_erase_, 
NAND_PAGE_SIZE)(NANDFlashState *s)
  }

  static bool glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
-                uint64_t addr, int offset)
+                uint64_t addr, unsigned offset)
  {
      if (PAGE(addr) >= s->pages) {
          return false;
---

> 
> Philippe Mathieu-Daudé (3):
>    hw/block/nand: Factor nand_load_iolen() method out
>    hw/block/nand: Have blk_load() take unsigned offset and return boolean
>    hw/block/nand: Fix out-of-bound access in NAND block buffer
> 
>   hw/block/nand.c | 55 ++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 38 insertions(+), 17 deletions(-)
>
Kevin Wolf April 9, 2024, 2:18 p.m. UTC | #2 
Am 09.04.2024 um 15:59 hat Philippe Mathieu-Daudé geschrieben:
> Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446
> 
> Since v1:
> - Addressed Kevin trivial suggestions (unsigned offset)

You already kept the Reviewed-by tags, but looks good to me.

Kevin
Philippe Mathieu-Daudé April 9, 2024, 2:31 p.m. UTC | #3 
On 9/4/24 16:18, Kevin Wolf wrote:
> Am 09.04.2024 um 15:59 hat Philippe Mathieu-Daudé geschrieben:
>> Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446
>>
>> Since v1:
>> - Addressed Kevin trivial suggestions (unsigned offset)
> 
> You already kept the Reviewed-by tags, but looks good to me.

Less work on your side ;)

The changes seemed trivial enough to keep them, but better
be safe than sorry.

Thanks!

Series queued.