Re: [Qemu-devel] [PATCH] [m25p80] Abort in case we overrun the internal data buffer

[Peter Maydell <peter.maydell@linaro.org>]

On 3 January 2017 at 21:17, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
>  hw/block/m25p80.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index d29ff4c..6c374cf 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -28,6 +28,7 @@
>  #include "hw/ssi/ssi.h"
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>
>  #ifndef M25P80_ERR_DEBUG
> @@ -376,6 +377,8 @@ typedef enum {
>      MAN_GENERIC,
>  } Manufacturer;
>
> +#define _INTERNAL_DATA_SIZE 16
> +

Don't use leading underscores, please.

>  typedef struct Flash {
>      SSISlave parent_obj;
>
> @@ -386,7 +389,7 @@ typedef struct Flash {
>      int page_size;
>
>      uint8_t state;
> -    uint8_t data[16];
> +    uint8_t data[_INTERNAL_DATA_SIZE];
>      uint32_t len;
>      uint32_t pos;
>      uint8_t needed_bytes;
> @@ -1114,6 +1117,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>
>      case STATE_COLLECTING_DATA:
>      case STATE_COLLECTING_VAR_LEN_DATA:
> +
> +        if (s->len >= _INTERNAL_DATA_SIZE) {
> +            error_report("Bug - Write overrun internal data buffer");
> +            abort();
> +        }
> +
>          s->data[s->len] = (uint8_t)tx;
>          s->len++;
>
> @@ -1123,6 +1132,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>          break;
>
>      case STATE_READING_DATA:
> +
> +        if (s->pos >= _INTERNAL_DATA_SIZE) {
> +            error_report("Bug - Read overrun internal data buffer");
> +            abort();
> +        }
> +

If these are "can't happen unless some other part of QEMU
is buggy" cases, then we can just assert():

    assert(s->pos < ARRAY_SIZE(s->data));

A comment about what kind of other part of QEMU might be buggy
if the assertion fires would also be helpful for future readers.

(If they're "could happen if the guest does something wrong"
cases, we shouldn't just abort(), but if I'm reading the previous
mail thread correctly, that's not the situation here.)

>          r = s->data[s->pos];
>          s->pos++;
>          if (s->pos == s->len) {
> @@ -1195,7 +1210,7 @@ static const VMStateDescription vmstate_m25p80 = {
>      .pre_save = m25p80_pre_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(state, Flash),
> -        VMSTATE_UINT8_ARRAY(data, Flash, 16),
> +        VMSTATE_UINT8_ARRAY(data, Flash, _INTERNAL_DATA_SIZE),
>          VMSTATE_UINT32(len, Flash),
>          VMSTATE_UINT32(pos, Flash),
>          VMSTATE_UINT8(needed_bytes, Flash),
> --
> 2.9.3

thanks
-- PMM