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

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>

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.