[prev in list] [next in list] [prev in thread] [next in thread]
List: qemu-devel
Subject: Re: [PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()
From: Philippe_Mathieu-Daudé <philmd () linaro ! org>
Date: 2024-03-25 10:26:09
Message-ID: aaaad221-9a88-4298-ac87-49d8a827ce9f () linaro ! org
[Download RAW message or body]
On 24/3/24 20:17, Mark Cave-Ayland wrote:
> During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset
> will always indicate the start of the SCSI CDB. However it is possible that a
> malicious guest could issue an invalid ESP command sequence such that cmdfifo
> wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO
> data buffer.
>
> Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped
> internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to
> access data outside the cmdfifo data buffer.
>
> Reported-by: Chuhong Yuan <hslester96@gmail.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/scsi/esp.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index f47abc36d6..d8db33b921 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
> {
> int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
> const uint8_t *pbuf;
> + uint32_t n;
> int cdblen;
>
> if (len <= 0) {
> return false;
> }
>
> - pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
> + pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n);
> + if (n < len) {
> + /*
> + * In normal use the cmdfifo should never wrap, but include this check
> + * to prevent a malicious guest from reading past the end of the
> + * cmdfifo data buffer below
> + */
Can we qemu_log_mask(LOG_GUEST_ERROR) something here?
> + return false;
> + }
> +
> cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
>
> return cdblen < 0 ? false : (len >= cdblen);
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic