Re: [PATCH] hw/scsi/esp: fix assertion error in fifo8_push

[Fabiano Rosas <farosas@suse.de>]

Zheng Huang <hz1624917200@gmail.com> writes:

> On 2025/5/28 03:40, Mark Cave-Ayland wrote:
>> On 27/05/2025 14:59, Philippe Mathieu-Daudé wrote:
>> 
>>> Hi,
>>>
>>> Cc'ing maintainers:
>>>
>>> $ ./scripts/get_maintainer.pl -f hw/scsi/esp.c
>>> Paolo Bonzini <pbonzini@redhat.com> (supporter:SCSI)
>>> Fam Zheng <fam@euphon.net> (reviewer:SCSI)
>>> $ ./scripts/get_maintainer.pl -f migration/
>>> Peter Xu <peterx@redhat.com> (maintainer:Migration)
>>> Fabiano Rosas <farosas@suse.de> (maintainer:Migration)
>>>
>>> On 27/5/25 15:12, Zheng Huang wrote:
>>>> This patch add validation checks on FIFO structures in esp_post_load() to
>>>> avoid assertion error `assert(fifo->num < fifo->capacity);` in fifo8_push(),
>>>> which can occur if the inbound migration stream is malformed. By performing
>>>> these checks during post-load, we can catch and handle such issues earlier,
>>>> avoiding crashes due to corrupted state.
>>>
>>> How can that happen? Can you share a reproducer?
>>>
>>>>
>>>> Signed-off-by: Zheng Huang <hz1624917200@gmail.com>
>>>> ---
>>>>   hw/scsi/esp.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>>> index ac841dc32e..ba77017087 100644
>>>> --- a/hw/scsi/esp.c
>>>> +++ b/hw/scsi/esp.c
>>>> @@ -1350,11 +1350,17 @@ static int esp_post_load(void *opaque, int version_id)
>>>>           /* Migrate ti_buf to fifo */
>>>>           len = s->mig_ti_wptr - s->mig_ti_rptr;
>>>>           for (i = 0; i < len; i++) {
>>>> +            if (&s->fifo.num >= &s->fifo.capacity) {
>>>> +                return -1;
>>>> +            }
>>>>               fifo8_push(&s->fifo, s->mig_ti_buf[i]);
>>>>           }
>>>>           /* Migrate cmdbuf to cmdfifo */
>>>>           for (i = 0; i < s->mig_cmdlen; i++) {
>>>> +            if (&s->cmdfifo.num >= &s->cmdfifo.capacity) {
>>>> +                return -1;
>>>> +            }
>>>>               fifo8_push(&s->cmdfifo, s->mig_cmdbuf[i]);
>>>>           }
>>>>       }
>> 
>> This seems odd: this logic in esp_post_load() is for converting from pre-Fifo8 code to the current Fifo8 code, so why wouldn't we want to assert() for the case when the migration stream is intentionally malformed? Is there a case whereby the old code could generate an invalid migration stream like this?
>> 
>> 
>> ATB,
>> 
>> Mark.
>> 
>
> Hi Mark,
>
> The malformed migration stream in question originates from QEMU itself—either accidentally, due to 
> a bug, or maliciously crafted. If we allow unchecked data through in esp_post_load(), an attacker
> controlling the migration source could send crafted values that trigger undefined behavior.
> The commit https://gitlab.com/qemu-project/qemu/-/commit/b88cfee90268cad376682da8f99ccf024d7aa304
> also check the migration stream integrity in post_load handler, which is suggested by Peter Maydell in
> https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg00099.html, 'to prevent the division-by-zero
> in the "malicious inbound migration state" case'.
>
> Also, I would appreciate your opinion on how we should handle such "malformed migration stream" case
> more generally, if there are more severe issues than assertion error, such as FPE, UAF, etc.? Should
> QEMU adopt a more systematic “post_load” validation pattern—verifying all critical fields across every
> migration handler—to harden the migration subsystem against any tampering of the migration image?
>

From the migration perspective it does make sense to validate the values
and return error. The migration stream should indeed be considered
untrusted.

But I agree that it would be nice to have some sort of reproducer. I
don't think it's practical to go around adding code to handle every
single hypothetical scenario. That creates some churn in the codebase
that might introduce bugs by itself.