[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 for-6.0 05/12] esp: introduce esp_fifo_pop_buf() and use i
From: |
Peter Maydell |
Subject: |
Re: [PATCH v4 for-6.0 05/12] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf() |
Date: |
Mon, 12 Apr 2021 11:56:59 +0100 |
On Wed, 7 Apr 2021 at 21:02, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> The const pointer returned by fifo8_pop_buf() lies directly within the array
> used
> to model the FIFO. Building with address sanitizers enabled shows that if the
> caller expects a minimum number of bytes present then if the FIFO is nearly
> full,
> the caller may unexpectedly access past the end of the array.
>
> Introduce esp_fifo_pop_buf() which takes a destination buffer and performs a
> memcpy() in it to guarantee that the caller cannot overwrite the FIFO array
> and
> update all callers to use it. Similarly add underflow protection similar to
> esp_fifo_push() and esp_fifo_pop() so that instead of triggering an assert()
> the operation becomes a no-op.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> ---
> hw/scsi/esp.c | 40 ++++++++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index ff8fa73de9..1aa2caf57d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -117,6 +117,23 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
> return fifo8_pop(fifo);
> }
>
> +static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
> +{
> + const uint8_t *buf;
> + uint32_t n;
> +
> + if (maxlen == 0) {
> + return 0;
> + }
> +
> + buf = fifo8_pop_buf(fifo, maxlen, &n);
> + if (dest) {
> + memcpy(dest, buf, n);
> + }
> +
> + return n;
> +}
> +
> static uint32_t esp_get_tc(ESPState *s)
> {
> uint32_t dmalen;
> @@ -241,11 +258,11 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
> if (dmalen == 0) {
> return 0;
> }
> - memcpy(buf, fifo8_pop_buf(&s->fifo, dmalen, &n), dmalen);
> - if (dmalen >= 3) {
> + n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
> + if (n >= 3) {
> buf[0] = buf[2] >> 5;
> }
> - fifo8_push_all(&s->cmdfifo, buf, dmalen);
> + fifo8_push_all(&s->cmdfifo, buf, n);
> }
> trace_esp_get_cmd(dmalen, target);
This bit could be tidied up further -- it currently sets
dmalen = MIN(fifo8_num_used(&s->fifo), maxlen);
in order not to pull more bytes out of the FIFO than it has in it;
but now we are making that check in esp_fifo_pop_buf() I think you
could just do
dmalen = esp_fifo_pop_buf(&s->fifo, buf, maxlen);
and drop the
dmalen = MIN(fifo8_num_used(&s->fifo), maxlen);
if (dmalen == 0) {
return 0;
}
part and the use of 'n' entirely.
But this code isn't wrong, so we can do that later to avoid having
to respin here.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
- [PATCH v4 for-6.0 00/12] esp: fix asserts/segfaults discovered by fuzzer, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 01/12] esp: always check current_req is not NULL before use in DMA callbacks, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 02/12] esp: rework write_response() to avoid using the FIFO for DMA transactions, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 03/12] esp: consolidate esp_cmdfifo_push() into esp_fifo_push(), Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 04/12] esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop(), Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 05/12] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf(), Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 06/12] esp: ensure cmdfifo is not empty and current_dev is non-NULL, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 07/12] esp: don't underflow cmdfifo in do_cmd(), Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 08/12] esp: don't overflow cmdfifo in get_cmd(), Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 09/12] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 10/12] esp: don't reset async_len directly in esp_select() if cancelling request, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 11/12] esp: ensure that do_cmd is set to zero before submitting an ESP select command, Mark Cave-Ayland, 2021/04/07
- [PATCH v4 for-6.0 12/12] tests/qtest: add tests for am53c974 device, Mark Cave-Ayland, 2021/04/07