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

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

[Jean-Christophe Dubois]

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
+
 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();
+        }
+
         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

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

[Peter Maydell]

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

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

[mar.krzeminski]

Hi Peter,

W dniu 05.01.2017 o 19:38, Peter Maydell pisze:
> 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.)
Indeed this case is about error in Qemu itself, but the same situation could
be generated from the guest (guest deasert CS only once).
IMHO we should reset m26p80 state in such case:
s->len = 0;
s->pos = 0;
s->state = STATE_IDLE;
This will be a bit closer to real HW behaviour too.

Thanks,
Marcin

>
>>           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
>

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

[Jean-Christophe DUBOIS]

Le 05/01/2017 à 21:04, mar.krzeminski a écrit :
> Hi Peter,
>
> W dniu 05.01.2017 o 19:38, Peter Maydell pisze:
>> 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.)
> Indeed this case is about error in Qemu itself, but the same situation 
> could
> be generated from the guest (guest deasert CS only once).
> IMHO we should reset m26p80 state in such case:
> s->len = 0;
> s->pos = 0;
> s->state = STATE_IDLE;
> This will be a bit closer to real HW behaviour too.

So what would be the preferred behavior?

  * Asserting (and ending Qemu)
  * Resetting (and hiding the misbehavior).

JC


>
> Thanks,
> Marcin
>
>>
>>>           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
>>
>
>

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

[Peter Maydell]

On 5 January 2017 at 20:18, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> Le 05/01/2017 à 21:04, mar.krzeminski a écrit :
>> Peter Maydell wrote:
>>> If these are "can't happen unless some other part of QEMU
>>> is buggy" cases, then we can just assert():

>>> (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.)
>
>> Indeed this case is about error in Qemu itself, but the same situation could
>> be generated from the guest (guest deasert CS only once).
>> IMHO we should reset m26p80 state in such case:
>> s->len = 0;
>> s->pos = 0;
>> s->state = STATE_IDLE;
>> This will be a bit closer to real HW behaviour too.

> So what would be the preferred behavior?
>
> Asserting (and ending Qemu)
> Resetting (and hiding the misbehavior).

If the guest can trigger this behaviour, then we should
not assert or abort or otherwise cause QEMU to exit.
The preferred behaviour is:
 * act like the real hardware does in this situation
   (whatever that is)
 * if this is something that only broken guest code would
   do, log it with qemu_log_mask(LOG_GUEST_ERROR, ...)

thanks
-- PMM

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

[Jean-Christophe DUBOIS]

Le 05/01/2017 à 21:51, Peter Maydell a écrit :
>> So what would be the preferred behavior?
>>
>> Asserting (and ending Qemu)
>> Resetting (and hiding the misbehavior).
> If the guest can trigger this behaviour, then we should
> not assert or abort or otherwise cause QEMU to exit.
> The preferred behaviour is:
>   * act like the real hardware does in this situation
>     (whatever that is)
>   * if this is something that only broken guest code would
>     do, log it with qemu_log_mask(LOG_GUEST_ERROR, ...)

I guess a real SPI controllers should not trigger this behavior like I 
did in the i.MX SPI controller emulation. It was a bug in my code and 
the crash (SIGSEGV) forced me to try to find a solution. At first I 
tried to fix the SPI device instead of the SPI controller because a 
SIGSEGV did not seem like an expected reaction and I was missing hints 
that my code was misbehaving.

But it is also possible for a guest to emulates the SPI controller 
through bit banging (for example). And in this case the guest could end 
up misbehaving in its software implementation.

So I think this behavior could be triggered either by buggy SPI 
controller emulator or by buggy guest software. And it seems hard to 
determine where the fault comes from from within Qemu. Obviously if the 
fault is in a Qemu emulator we would not want to tag the error as a 
LOG_GUEST_ERROR. On the other hand, it would be nice to warn the user if 
the guest if it is indeed misbehaving so that he can fix his code.

>
> thanks
> -- PMM
>
>

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

[Peter Maydell]

On 5 January 2017 at 21:39, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> So I think this behavior could be triggered either by buggy SPI controller
> emulator or by buggy guest software. And it seems hard to determine where
> the fault comes from from within Qemu. Obviously if the fault is in a Qemu
> emulator we would not want to tag the error as a LOG_GUEST_ERROR. On the
> other hand, it would be nice to warn the user if the guest if it is indeed
> misbehaving so that he can fix his code.

We should log it as a guest error. "warning might be wrong if there's
a bug in QEMU" is true of pretty much any warning we emit...

thanks
-- PMM

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

[Jean-Christophe DUBOIS]

Le 06/01/2017 à 11:18, Peter Maydell a écrit :
> On 5 January 2017 at 21:39, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>> So I think this behavior could be triggered either by buggy SPI controller
>> emulator or by buggy guest software. And it seems hard to determine where
>> the fault comes from from within Qemu. Obviously if the fault is in a Qemu
>> emulator we would not want to tag the error as a LOG_GUEST_ERROR. On the
>> other hand, it would be nice to warn the user if the guest if it is indeed
>> misbehaving so that he can fix his code.
> We should log it as a guest error. "warning might be wrong if there's
> a bug in QEMU" is true of pretty much any warning we emit...

OK, I'll do this.

JC

>
> thanks
> -- PMM
>