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

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

[Philippe Mathieu-Daudé]

Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446

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

 hw/block/nand.c | 50 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

-- 
2.41.0

[PATCH-for-9.0? 1/3] hw/block/nand: Factor nand_load_iolen() method out

[Philippe Mathieu-Daudé]

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);
+
+    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) {
-- 
2.41.0

Re: [PATCH-for-9.0? 1/3] hw/block/nand: Factor nand_load_iolen() method out

[Richard Henderson]

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~

Re: [PATCH-for-9.0? 1/3] hw/block/nand: Factor nand_load_iolen() method out

[Kevin Wolf]

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

[PATCH-for-9.0? 2/3] hw/block/nand: Have blk_load() return boolean indicating success

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/nand.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 6fa9038bb5..3627c799b5 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -84,7 +84,11 @@ struct NANDFlashState {
 
     void (*blk_write)(NANDFlashState *s);
     void (*blk_erase)(NANDFlashState *s);
-    void (*blk_load)(NANDFlashState *s, uint64_t addr, int offset);
+    /*
+     * Returns %true when block containing (@addr + @offset) is
+     * successfully loaded, otherwise %false.
+     */
+    bool (*blk_load)(NANDFlashState *s, uint64_t addr, int offset);
 
     uint32_t ioaddr_vmstate;
 };
@@ -769,11 +773,11 @@ static void glue(nand_blk_erase_, NAND_PAGE_SIZE)(NANDFlashState *s)
     }
 }
 
-static void glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
+static bool glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
                 uint64_t addr, int offset)
 {
     if (PAGE(addr) >= s->pages) {
-        return;
+        return false;
     }
 
     if (s->blk) {
@@ -801,6 +805,8 @@ static void glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
                         offset, NAND_PAGE_SIZE + OOB_SIZE - offset);
         s->ioaddr = s->io;
     }
+
+    return true;
 }
 
 static void glue(nand_init_, NAND_PAGE_SIZE)(NANDFlashState *s)
-- 
2.41.0

Re: [PATCH-for-9.0? 2/3] hw/block/nand: Have blk_load() return boolean indicating success

[Richard Henderson]

On 4/7/24 22:36, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/block/nand.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Philippe Mathieu-Daudé]

nand_command() and nand_getio() don't check @offset points
into the block, nor the available data length (s->iolen) is
not negative.

In order to fix:

- check the offset is in range in nand_blk_load_NAND_PAGE_SIZE(),
- do not set @iolen if blk_load() failed.

Reproducer:

  $ cat << EOF | qemu-system-arm -machine tosa \
                                 -monitor none -serial none \
                                 -display none -qtest stdio
  write 0x10000111 0x1 0xca
  write 0x10000104 0x1 0x47
  write 0x1000ca04 0x1 0xd7
  write 0x1000ca01 0x1 0xe0
  write 0x1000ca04 0x1 0x71
  write 0x1000ca00 0x1 0x50
  write 0x1000ca04 0x1 0xd7
  read 0x1000ca02 0x1
  write 0x1000ca01 0x1 0x10
  EOF

=================================================================
==15750==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61f000000de0
 at pc 0x560e61557210 bp 0x7ffcfc4a59f0 sp 0x7ffcfc4a59e8
READ of size 1 at 0x61f000000de0 thread T0
    #0 0x560e6155720f in mem_and hw/block/nand.c:101:20
    #1 0x560e6155ac9c in nand_blk_write_512 hw/block/nand.c:663:9
    #2 0x560e61544200 in nand_command hw/block/nand.c:293:13
    #3 0x560e6153cc83 in nand_setio hw/block/nand.c:520:13
    #4 0x560e61a0a69e in tc6393xb_nand_writeb hw/display/tc6393xb.c:380:13
    #5 0x560e619f9bf7 in tc6393xb_writeb hw/display/tc6393xb.c:524:9
    #6 0x560e647c7d03 in memory_region_write_accessor softmmu/memory.c:492:5
    #7 0x560e647c7641 in access_with_adjusted_size softmmu/memory.c:554:18
    #8 0x560e647c5f66 in memory_region_dispatch_write softmmu/memory.c:1514:16
    #9 0x560e6485409e in flatview_write_continue softmmu/physmem.c:2825:23
    #10 0x560e648421eb in flatview_write softmmu/physmem.c:2867:12
    #11 0x560e64841ca8 in address_space_write softmmu/physmem.c:2963:18
    #12 0x560e61170162 in qemu_writeb tests/qtest/videzzo/videzzo_qemu.c:1080:5
    #13 0x560e6116eef7 in dispatch_mmio_write tests/qtest/videzzo/videzzo_qemu.c:1227:28

0x61f000000de0 is located 0 bytes to the right of 3424-byte region [0x61f000000080,0x61f000000de0)
allocated by thread T0 here:
    #0 0x560e611276cf in malloc /root/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x7f7959a87e98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
    #2 0x560e64b98871 in object_new qom/object.c:749:12
    #3 0x560e64b5d1a1 in qdev_new hw/core/qdev.c:153:19
    #4 0x560e61547ea5 in nand_init hw/block/nand.c:639:11
    #5 0x560e619f8772 in tc6393xb_init hw/display/tc6393xb.c:558:16
    #6 0x560e6390bad2 in tosa_init hw/arm/tosa.c:250:12

SUMMARY: AddressSanitizer: heap-buffer-overflow hw/block/nand.c:101:20 in mem_and
==15750==ABORTING

Broken since introduction in commit 3e3d5815cb ("NAND Flash memory
emulation and ECC calculation helpers for use by NAND controllers").

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1446
Reported-by: Qiang Liu <cyruscyliu@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/nand.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 3627c799b5..d90dc965a1 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -255,7 +255,9 @@ static int nand_load_block(NANDFlashState *s, int offset)
 {
     int iolen;
 
-    s->blk_load(s, s->addr, offset);
+    if (!s->blk_load(s, s->addr, offset)) {
+        return 0;
+    }
 
     iolen = (1 << s->page_shift) - offset;
     if (s->gnd) {
@@ -780,6 +782,10 @@ static bool glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
         return false;
     }
 
+    if (offset > NAND_PAGE_SIZE + OOB_SIZE) {
+        return false;
+    }
+
     if (s->blk) {
         if (s->mem_oob) {
             if (blk_pread(s->blk, SECTOR(addr) << BDRV_SECTOR_BITS,
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

On 8/4/24 10:36, Philippe Mathieu-Daudé wrote:
> nand_command() and nand_getio() don't check @offset points
> into the block, nor the available data length (s->iolen) is
> not negative.
> 
> In order to fix:
> 
> - check the offset is in range in nand_blk_load_NAND_PAGE_SIZE(),
> - do not set @iolen if blk_load() failed.
> 
> Reproducer:
> 
>    $ cat << EOF | qemu-system-arm -machine tosa \
>                                   -monitor none -serial none \
>                                   -display none -qtest stdio
>    write 0x10000111 0x1 0xca
>    write 0x10000104 0x1 0x47
>    write 0x1000ca04 0x1 0xd7
>    write 0x1000ca01 0x1 0xe0
>    write 0x1000ca04 0x1 0x71
>    write 0x1000ca00 0x1 0x50
>    write 0x1000ca04 0x1 0xd7
>    read 0x1000ca02 0x1
>    write 0x1000ca01 0x1 0x10
>    EOF
> 
> =================================================================
> ==15750==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61f000000de0
>   at pc 0x560e61557210 bp 0x7ffcfc4a59f0 sp 0x7ffcfc4a59e8
> READ of size 1 at 0x61f000000de0 thread T0
>      #0 0x560e6155720f in mem_and hw/block/nand.c:101:20
>      #1 0x560e6155ac9c in nand_blk_write_512 hw/block/nand.c:663:9
>      #2 0x560e61544200 in nand_command hw/block/nand.c:293:13
>      #3 0x560e6153cc83 in nand_setio hw/block/nand.c:520:13
>      #4 0x560e61a0a69e in tc6393xb_nand_writeb hw/display/tc6393xb.c:380:13
>      #5 0x560e619f9bf7 in tc6393xb_writeb hw/display/tc6393xb.c:524:9
>      #6 0x560e647c7d03 in memory_region_write_accessor softmmu/memory.c:492:5
>      #7 0x560e647c7641 in access_with_adjusted_size softmmu/memory.c:554:18
>      #8 0x560e647c5f66 in memory_region_dispatch_write softmmu/memory.c:1514:16
>      #9 0x560e6485409e in flatview_write_continue softmmu/physmem.c:2825:23
>      #10 0x560e648421eb in flatview_write softmmu/physmem.c:2867:12
>      #11 0x560e64841ca8 in address_space_write softmmu/physmem.c:2963:18
>      #12 0x560e61170162 in qemu_writeb tests/qtest/videzzo/videzzo_qemu.c:1080:5
>      #13 0x560e6116eef7 in dispatch_mmio_write tests/qtest/videzzo/videzzo_qemu.c:1227:28
> 
> 0x61f000000de0 is located 0 bytes to the right of 3424-byte region [0x61f000000080,0x61f000000de0)
> allocated by thread T0 here:
>      #0 0x560e611276cf in malloc /root/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
>      #1 0x7f7959a87e98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
>      #2 0x560e64b98871 in object_new qom/object.c:749:12
>      #3 0x560e64b5d1a1 in qdev_new hw/core/qdev.c:153:19
>      #4 0x560e61547ea5 in nand_init hw/block/nand.c:639:11
>      #5 0x560e619f8772 in tc6393xb_init hw/display/tc6393xb.c:558:16
>      #6 0x560e6390bad2 in tosa_init hw/arm/tosa.c:250:12
> 
> SUMMARY: AddressSanitizer: heap-buffer-overflow hw/block/nand.c:101:20 in mem_and
> ==15750==ABORTING
> 
> Broken since introduction in commit 3e3d5815cb ("NAND Flash memory
> emulation and ECC calculation helpers for use by NAND controllers").
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1446

Also:

   Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1445

> Reported-by: Qiang Liu <cyruscyliu@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/block/nand.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

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

[Richard Henderson]

On 4/7/24 22:36, Philippe Mathieu-Daudé wrote:
> nand_command() and nand_getio() don't check @offset points
> into the block, nor the available data length (s->iolen) is
> not negative.
> 
> In order to fix:
> 
> - check the offset is in range in nand_blk_load_NAND_PAGE_SIZE(),
> - do not set @iolen if blk_load() failed.

Do not set, or do not set to non-zero?  I had been wondering if the final assignment to 
s->iolen should go into nand_load_block as well...

> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 3627c799b5..d90dc965a1 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -255,7 +255,9 @@ static int nand_load_block(NANDFlashState *s, int offset)
>   {
>       int iolen;
>   
> -    s->blk_load(s, s->addr, offset);
> +    if (!s->blk_load(s, s->addr, offset)) {
> +        return 0;
> +    }
>   
>       iolen = (1 << s->page_shift) - offset;
>       if (s->gnd) {
> @@ -780,6 +782,10 @@ static bool glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
>           return false;
>       }
>   
> +    if (offset > NAND_PAGE_SIZE + OOB_SIZE) {
> +        return false;
> +    }
> +

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[Philippe Mathieu-Daudé]

On 8/4/24 18:39, Richard Henderson wrote:
> On 4/7/24 22:36, Philippe Mathieu-Daudé wrote:
>> nand_command() and nand_getio() don't check @offset points
>> into the block, nor the available data length (s->iolen) is
>> not negative.
>>
>> In order to fix:
>>
>> - check the offset is in range in nand_blk_load_NAND_PAGE_SIZE(),
>> - do not set @iolen if blk_load() failed.
> 
> Do not set, or do not set to non-zero?  I had been wondering if the 

Oh, "do not set to non-zero", thanks :)

> final assignment to s->iolen should go into nand_load_block as well...

For the next tag I rather keep it this way which seems more
explicit to me.

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

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

[Mauro Matteo Cascella]

On Mon, Apr 8, 2024 at 10:36 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446

Does hw/block/nand meet the security requirements for CVE assignment?

=> https://www.qemu.org/docs/master/system/security.html

> Philippe Mathieu-Daudé (3):
>   hw/block/nand: Factor nand_load_iolen() method out
>   hw/block/nand: Have blk_load() return boolean indicating success
>   hw/block/nand: Fix out-of-bound access in NAND block buffer
>
>  hw/block/nand.c | 50 +++++++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 16 deletions(-)
>
> --
> 2.41.0
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

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

[Philippe Mathieu-Daudé]

On 8/4/24 17:45, Mauro Matteo Cascella wrote:
> On Mon, Apr 8, 2024 at 10:36 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446
> 
> Does hw/block/nand meet the security requirements for CVE assignment?
> 
> => https://www.qemu.org/docs/master/system/security.html

I don't think this device model is used in virtualization,
so I don't think so. (Cc'ing qemu-arm@ in case).
Thanks!

> 
>> Philippe Mathieu-Daudé (3):
>>    hw/block/nand: Factor nand_load_iolen() method out
>>    hw/block/nand: Have blk_load() return boolean indicating success
>>    hw/block/nand: Fix out-of-bound access in NAND block buffer
>>
>>   hw/block/nand.c | 50 +++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 34 insertions(+), 16 deletions(-)
>>
>> --
>> 2.41.0
>>
> 

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

[Kevin Wolf]

Am 08.04.2024 um 10:36 hat Philippe Mathieu-Daudé geschrieben:
> Fix for https://gitlab.com/qemu-project/qemu/-/issues/1446
> 
> Philippe Mathieu-Daudé (3):
>   hw/block/nand: Factor nand_load_iolen() method out
>   hw/block/nand: Have blk_load() return boolean indicating success
>   hw/block/nand: Fix out-of-bound access in NAND block buffer

As we're short on time for 9.0:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

But it feels to me like this device could use some more cleanup to make
the code more robust.

Kevin