The const pointer returned by fifo8_pop_buf() lies directly within the array used
to model the FIFO. Building with address sanitisers 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>
---
hw/scsi/esp.c | 41 +++++++++++++++++++++++++++++------------
1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ce88866803..1aa2caf57d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -107,6 +107,7 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
fifo8_push(fifo, val);
}
+
static uint8_t esp_fifo_pop(Fifo8 *fifo)
{
if (fifo8_is_empty(fifo)) {
@@ -116,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;
@@ -240,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);
@@ -257,16 +275,16 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
static void do_busid_cmd(ESPState *s, uint8_t busid)
{
- uint32_t n, cmdlen;
+ uint32_t cmdlen;
int32_t datalen;
int lun;
SCSIDevice *current_lun;
- uint8_t *buf;
+ uint8_t buf[ESP_CMDFIFO_SZ];
trace_esp_do_busid_cmd(busid);
lun = busid & 7;
cmdlen = fifo8_num_used(&s->cmdfifo);
- buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n);
+ esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
@@ -299,13 +317,12 @@ static void do_busid_cmd(ESPState *s, uint8_t busid)
static void do_cmd(ESPState *s)
{
uint8_t busid = fifo8_pop(&s->cmdfifo);
- uint32_t n;
s->cmdfifo_cdb_offset--;
/* Ignore extended messages for now */
if (s->cmdfifo_cdb_offset) {
- fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n);
+ esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset);
s->cmdfifo_cdb_offset = 0;
}
@@ -483,7 +500,7 @@ static void do_dma_pdma_cb(ESPState *s)
/* Copy FIFO data to device */
len = MIN(s->async_len, ESP_FIFO_SZ);
len = MIN(len, fifo8_num_used(&s->fifo));
- memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len);
+ n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
s->async_buf += n;
s->async_len -= n;
s->ti_size += n;
@@ -491,7 +508,7 @@ static void do_dma_pdma_cb(ESPState *s)
if (n < len) {
/* Unaligned accesses can cause FIFO wraparound */
len = len - n;
- memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len);
+ n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
s->async_buf += n;
s->async_len -= n;
s->ti_size += n;
@@ -667,7 +684,7 @@ static void esp_do_dma(ESPState *s)
static void esp_do_nodma(ESPState *s)
{
int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
- uint32_t cmdlen, n;
+ uint32_t cmdlen;
int len;
if (s->do_cmd) {
@@ -708,7 +725,7 @@ static void esp_do_nodma(ESPState *s)
if (to_device) {
len = MIN(fifo8_num_used(&s->fifo), ESP_FIFO_SZ);
- memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len);
+ esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
s->async_buf += len;
s->async_len -= len;
s->ti_size += len;
--
2.20.1
On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> The const pointer returned by fifo8_pop_buf() lies directly within the array used
> to model the FIFO. Building with address sanitisers enabled shows that if the
Typo "sanitizers"
> 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.
Why isn't it a problem with the other models? Because the pointed
buffer is consumed directly?
> 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.
This is OK for your ESP model.
Now thinking loudly about the Fifo8 API, shouldn't this be part of it?
Something prototype like:
/**
* fifo8_pop_buf:
* @do_copy: If %true, also copy data to @bufptr.
*/
size_t fifo8_pop_buf(Fifo8 *fifo,
void **bufptr,
size_t buflen,
bool do_copy);
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/scsi/esp.c | 41 +++++++++++++++++++++++++++++------------
> 1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index ce88866803..1aa2caf57d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -107,6 +107,7 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
>
> fifo8_push(fifo, val);
> }
> +
> static uint8_t esp_fifo_pop(Fifo8 *fifo)
> {
> if (fifo8_is_empty(fifo)) {
> @@ -116,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;
> +}
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 01/04/2021 10:34, Philippe Mathieu-Daudé wrote: > On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: >> The const pointer returned by fifo8_pop_buf() lies directly within the array used >> to model the FIFO. Building with address sanitisers enabled shows that if the > > Typo "sanitizers" Ha. It's definitely "sanitiser" here in the UK (UK English) as opposed to "sanitizer" (US English). I don't really mind either way, but I can fix this if it needs a v4 following Paolo's comments. >> 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. > > Why isn't it a problem with the other models? Because the pointed > buffer is consumed directly? Yes that's correct, which is why Fifo8 currently doesn't support wraparound. I haven't analysed how other devices have used it but I would imagine there would be an ASan hit if it were being misused this way. >> 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. > > This is OK for your ESP model. > > Now thinking loudly about the Fifo8 API, shouldn't this be part of it? > > Something prototype like: > > /** > * fifo8_pop_buf: > * @do_copy: If %true, also copy data to @bufptr. > */ > size_t fifo8_pop_buf(Fifo8 *fifo, > void **bufptr, > size_t buflen, > bool do_copy); That could work, and may even allow support for wraparound in future. I suspect things would become clearer after looking at the other Fifo8 users to see if this is worth an API change/alternative API. ATB, Mark.
On 4/1/21 12:51 PM, Mark Cave-Ayland wrote: > On 01/04/2021 10:34, Philippe Mathieu-Daudé wrote: >> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: >>> The const pointer returned by fifo8_pop_buf() lies directly within >>> the array used >>> to model the FIFO. Building with address sanitisers enabled shows >>> that if the >> >> Typo "sanitizers" > > Ha. It's definitely "sanitiser" here in the UK (UK English) as opposed > to "sanitizer" (US English). Oh OK, TIL :) > I don't really mind either way, but I can > fix this if it needs a v4 following Paolo's comments. Not needed since it is correct. >>> 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. >> >> Why isn't it a problem with the other models? Because the pointed >> buffer is consumed directly? > > Yes that's correct, which is why Fifo8 currently doesn't support > wraparound. I haven't analysed how other devices have used it but I > would imagine there would be an ASan hit if it were being misused this way. > >>> 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. >> >> This is OK for your ESP model. >> >> Now thinking loudly about the Fifo8 API, shouldn't this be part of it? >> >> Something prototype like: >> >> /** >> * fifo8_pop_buf: >> * @do_copy: If %true, also copy data to @bufptr. >> */ >> size_t fifo8_pop_buf(Fifo8 *fifo, >> void **bufptr, >> size_t buflen, >> bool do_copy); > > That could work, and may even allow support for wraparound in future. I > suspect things would become clearer after looking at the other Fifo8 > users to see if this is worth an API change/alternative API. Thanks for the feedback! Phil.
© 2016 - 2026 Red Hat, Inc.