[PATCH 0/2] esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation

[PATCH 0/2] esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation

[Mark Cave-Ayland]

This small series fixes a bug in the ESP cmdfifo FIFO wraparound limit
calculation as reported at https://gitlab.com/qemu-project/qemu/-/issues/3082, as
well as adding the reported test case to qtest.

In normal usage the cmdfifo does not overflow, however the test case reported by
the fuzzer was enough to show that the FIFO overflow check in esp_cdb_ready()
was not working correctly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (2):
  esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation
  qtest/am53c974-test: add additional test for cmdfifo overflow

 hw/scsi/esp.c               |  6 ++++--
 tests/qtest/am53c974-test.c | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

-- 
2.39.5

[PATCH 1/2] esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation

[Mark Cave-Ayland]

The original calculation in commit 3cc70889a3 ("esp.c: prevent cmdfifo overflow
in esp_cdb_ready()") subtracted cmdfifo_cdb_offset from fifo8_num_used() to
calculate the outstanding cmdfifo length, but this is incorrect because
fifo8_num_used() can also include wraparound data.

Instead calculate the maximum offset used by scsi_cdb_length() which is just
the first byte after cmdfifo_cdb_offset, and then peek the entire content
of the cmdfifo. The fifo8_peek_bufptr() result will then return the maximum
length of remaining data up to the end of the internal cmdfifo array, which
can then be used for the overflow check.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 3cc70889a3 ("esp.c: prevent cmdfifo overflow in esp_cdb_ready()")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3082
---
 hw/scsi/esp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1d264c40e5..2809fcdee0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -447,7 +447,9 @@ static void write_response(ESPState *s)
 
 static bool esp_cdb_ready(ESPState *s)
 {
-    int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
+    /* scsi_cdb_length() only reads the first byte */
+    int limit = s->cmdfifo_cdb_offset + 1;
+    int len = fifo8_num_used(&s->cmdfifo);
     const uint8_t *pbuf;
     uint32_t n;
     int cdblen;
@@ -457,7 +459,7 @@ static bool esp_cdb_ready(ESPState *s)
     }
 
     pbuf = fifo8_peek_bufptr(&s->cmdfifo, len, &n);
-    if (n < len) {
+    if (n < limit) {
         /*
          * In normal use the cmdfifo should never wrap, but include this check
          * to prevent a malicious guest from reading past the end of the
-- 
2.39.5

Re: [PATCH 1/2] esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation

[Philippe Mathieu-Daudé]

On 25/9/25 14:28, Mark Cave-Ayland wrote:
> The original calculation in commit 3cc70889a3 ("esp.c: prevent cmdfifo overflow
> in esp_cdb_ready()") subtracted cmdfifo_cdb_offset from fifo8_num_used() to

"substracted"

> calculate the outstanding cmdfifo length, but this is incorrect because
> fifo8_num_used() can also include wraparound data.
> 
> Instead calculate the maximum offset used by scsi_cdb_length() which is just
> the first byte after cmdfifo_cdb_offset, and then peek the entire content
> of the cmdfifo. The fifo8_peek_bufptr() result will then return the maximum
> length of remaining data up to the end of the internal cmdfifo array, which
> can then be used for the overflow check.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3cc70889a3 ("esp.c: prevent cmdfifo overflow in esp_cdb_ready()")

Buglink: https://issues.oss-fuzz.com/issues/439878564

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3082
> ---
>   hw/scsi/esp.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 1d264c40e5..2809fcdee0 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -447,7 +447,9 @@ static void write_response(ESPState *s)
>   
>   static bool esp_cdb_ready(ESPState *s)
>   {
> -    int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
> +    /* scsi_cdb_length() only reads the first byte */
> +    int limit = s->cmdfifo_cdb_offset + 1;
> +    int len = fifo8_num_used(&s->cmdfifo);
>       const uint8_t *pbuf;
>       uint32_t n;
>       int cdblen;
> @@ -457,7 +459,7 @@ static bool esp_cdb_ready(ESPState *s)
>       }
>   
>       pbuf = fifo8_peek_bufptr(&s->cmdfifo, len, &n);
> -    if (n < len) {
> +    if (n < limit) {
>           /*
>            * In normal use the cmdfifo should never wrap, but include this check
>            * to prevent a malicious guest from reading past the end of the
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation

[Mark Cave-Ayland]

On 25/09/2025 14:33, Philippe Mathieu-Daudé wrote:

> On 25/9/25 14:28, Mark Cave-Ayland wrote:
>> The original calculation in commit 3cc70889a3 ("esp.c: prevent cmdfifo overflow
>> in esp_cdb_ready()") subtracted cmdfifo_cdb_offset from fifo8_num_used() to
> 
> "substracted"

I had to look this one up, and I'm disappointed to report that it's considered 
obsolete these days ;)

>> calculate the outstanding cmdfifo length, but this is incorrect because
>> fifo8_num_used() can also include wraparound data.
>>
>> Instead calculate the maximum offset used by scsi_cdb_length() which is just
>> the first byte after cmdfifo_cdb_offset, and then peek the entire content
>> of the cmdfifo. The fifo8_peek_bufptr() result will then return the maximum
>> length of remaining data up to the end of the internal cmdfifo array, which
>> can then be used for the overflow check.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Fixes: 3cc70889a3 ("esp.c: prevent cmdfifo overflow in esp_cdb_ready()")
> 
> Buglink: https://issues.oss-fuzz.com/issues/439878564
> 
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3082
>> ---
>>   hw/scsi/esp.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 1d264c40e5..2809fcdee0 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -447,7 +447,9 @@ static void write_response(ESPState *s)
>>   static bool esp_cdb_ready(ESPState *s)
>>   {
>> -    int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
>> +    /* scsi_cdb_length() only reads the first byte */
>> +    int limit = s->cmdfifo_cdb_offset + 1;
>> +    int len = fifo8_num_used(&s->cmdfifo);
>>       const uint8_t *pbuf;
>>       uint32_t n;
>>       int cdblen;
>> @@ -457,7 +459,7 @@ static bool esp_cdb_ready(ESPState *s)
>>       }
>>       pbuf = fifo8_peek_bufptr(&s->cmdfifo, len, &n);
>> -    if (n < len) {
>> +    if (n < limit) {
>>           /*
>>            * In normal use the cmdfifo should never wrap, but include this check
>>            * to prevent a malicious guest from reading past the end of the
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks for the review!


ATB,

Mark.

[PATCH 2/2] qtest/am53c974-test: add additional test for cmdfifo overflow

[Mark Cave-Ayland]

Based upon the qtest reproducer posted to Gitlab issue #3082 at
https://gitlab.com/qemu-project/qemu/-/issues/3082.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 tests/qtest/am53c974-test.c | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
index ed3ac7db20..a3667275ac 100644
--- a/tests/qtest/am53c974-test.c
+++ b/tests/qtest/am53c974-test.c
@@ -109,6 +109,44 @@ static void test_cmdfifo_overflow2_ok(void)
     qtest_quit(s);
 }
 
+/* Reported as https://issues.oss-fuzz.com/issues/439878564 */
+static void test_cmdfifo_overflow3_ok(void)
+{
+    QTestState *s = qtest_init(
+        "-device am53c974,id=scsi -device scsi-hd,drive=disk0 "
+        "-drive id=disk0,if=none,file=null-co://,format=raw -nodefaults");
+    qtest_outl(s, 0xcf8, 0x80001010);
+    qtest_outl(s, 0xcfc, 0xc000);
+    qtest_outl(s, 0xcf8, 0x80001004);
+    qtest_outw(s, 0xcfc, 0x01);
+    qtest_outb(s, 0xc00c, 0x43);
+    qtest_outl(s, 0xc00b, 0x9100);
+    qtest_outl(s, 0xc009, 0x02000000);
+    qtest_outl(s, 0xc000, 0x0b);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outl(s, 0xc00b, 0xc200);
+    qtest_outl(s, 0xc00b, 0x1000);
+    qtest_outl(s, 0xc00b, 0x9000);
+    qtest_outb(s, 0xc008, 0x00);
+    qtest_outb(s, 0xc008, 0x00);
+    qtest_outl(s, 0xc03f, 0x0300);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x4200);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x1200);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outb(s, 0xc00c, 0x43);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outl(s, 0xc007, 0x00);
+    qtest_outl(s, 0xc007, 0x00);
+    qtest_outl(s, 0xc007, 0x00);
+    qtest_outl(s, 0xc00b, 0x1000);
+    qtest_outl(s, 0xc007, 0x00);
+    qtest_quit(s);
+}
+
 /* Reported as crash_0900379669 */
 static void test_fifo_pop_buf(void)
 {
@@ -266,6 +304,8 @@ int main(int argc, char **argv)
                        test_cmdfifo_overflow_ok);
         qtest_add_func("am53c974/test_cmdfifo_overflow2_ok",
                        test_cmdfifo_overflow2_ok);
+        qtest_add_func("am53c974/test_cmdfifo_overflow3_ok",
+                       test_cmdfifo_overflow3_ok);
         qtest_add_func("am53c974/test_fifo_pop_buf",
                        test_fifo_pop_buf);
         qtest_add_func("am53c974/test_target_selected_ok",
-- 
2.39.5

Re: [PATCH 2/2] qtest/am53c974-test: add additional test for cmdfifo overflow

[Philippe Mathieu-Daudé]

On 25/9/25 14:28, Mark Cave-Ayland wrote:
> Based upon the qtest reproducer posted to Gitlab issue #3082 at
> https://gitlab.com/qemu-project/qemu/-/issues/3082.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   tests/qtest/am53c974-test.c | 40 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/2] esp.c: fix esp_cdb_ready() FIFO wraparound limit calculation

[Paolo Bonzini]

Queued, thanks.

Paolo