[PATCH v1 0/2] hw/ppc: SPI model - coverity fixes

[PATCH v1 0/2] hw/ppc: SPI model - coverity fixes

[Chalapathi V]

Hello,

In this patchset below defects detected during coverity scan for latest
spi model changes were fixed.

- CID 1558831:  Resource leaks  (RESOURCE_LEAK)
  Variable "rsp_payload" going out of scope leaks the storage it points to.
- CID 1558827:    (OVERRUN)
  Overrunning array "s->seq_op" of 8 bytes at byte offset 16 using index
  "get_seq_index(s) + 1" (which evaluates to 16).

Tested:
passed make check and make check-avocado

Chalapathi V (2):
  Fixes: Coverity CID 1558827
  Fixes: Coverity CID 1558831

 hw/ssi/pnv_spi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] Fixes: Coverity CID 1558827

[Chalapathi V]

In this commit the following coverity scan defect has been fixed.
CID 1558827:    (OVERRUN)
  Overrunning array "s->seq_op" of 8 bytes at byte offset 16
using index "get_seq_index(s) + 1" (which evaluates to 16).

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 hw/ssi/pnv_spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index c1297ab733..a33f682897 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -729,7 +729,7 @@ static void operation_sequencer(PnvSpi *s)
      * some operations may cause more than one frame to be sequenced.
      */
     while (get_seq_index(s) < NUM_SEQ_OPS) {
-        opcode = s->seq_op[get_seq_index(s)];
+        opcode = s->seq_op[(get_seq_index(s) & 0x7)];
         /* Set sequencer state to decode */
         s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
         /*
@@ -834,8 +834,8 @@ static void operation_sequencer(PnvSpi *s)
                  * transmission to the responder without requiring a refill of
                  * the TDR between the two operations.
                  */
-                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
-                                == SEQ_OP_SHIFT_N2) {
+                if (PNV_SPI_MASKED_OPCODE(s->seq_op[((get_seq_index(s) + 1) &
+                                                0x7)]) == SEQ_OP_SHIFT_N2) {
                     send_n1_alone = false;
                 }
                 s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
-- 
2.34.1

[PATCH v1 2/2] Fixes: Coverity CID 1558831

[Chalapathi V]

In this commit the following coverity scan defect has been fixed
CID 1558831:  Resource leaks  (RESOURCE_LEAK)
  Variable "rsp_payload" going out of scope leaks the storage it
  points to.

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 hw/ssi/pnv_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index a33f682897..dbe06df224 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -237,6 +237,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
     }
     if (rsp_payload != NULL) {
         spi_response(s, s->N1_bits, rsp_payload);
+        pnv_spi_xfer_buffer_free(rsp_payload);
     }
 }
 
-- 
2.34.1

Re: [PATCH v1 1/2] Fixes: Coverity CID 1558827

[Philippe Mathieu-Daudé]

Hi Chalapathi,

Please prefix subject with "hw/ssi/pnv".

On 6/8/24 15:48, Chalapathi V wrote:
> In this commit the following coverity scan defect has been fixed.
> CID 1558827:    (OVERRUN)
>    Overrunning array "s->seq_op" of 8 bytes at byte offset 16
> using index "get_seq_index(s) + 1" (which evaluates to 16).
> 
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>   hw/ssi/pnv_spi.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index c1297ab733..a33f682897 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -729,7 +729,7 @@ static void operation_sequencer(PnvSpi *s)
>        * some operations may cause more than one frame to be sequenced.
>        */
>       while (get_seq_index(s) < NUM_SEQ_OPS) {
> -        opcode = s->seq_op[get_seq_index(s)];
> +        opcode = s->seq_op[(get_seq_index(s) & 0x7)];

seq_op[] has PNV_SPI_REG_SIZE elements, PNV_SPI_REG_SIZE being 8.

We also have NUM_SEQ_OPS defined as 8.

get_seq_index() returns SPI_STS_SEQ_INDEX. Being defined as
PPC_BITMASK(28, 31), it is 4-bit width. (I was wondering why
not have get_seq_index return a masked value).

I don't know this area, but this code is not very clear...

Alternative to make Coverity happy:

   seq_index = get_seq_index(s);
   assert(seq_index < NUM_SEQ_OPS);
   opcode = s->seq_op[seq_index];

>           /* Set sequencer state to decode */
>           s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
>           /*
> @@ -834,8 +834,8 @@ static void operation_sequencer(PnvSpi *s)
>                    * transmission to the responder without requiring a refill of
>                    * the TDR between the two operations.
>                    */
> -                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
> -                                == SEQ_OP_SHIFT_N2) {
> +                if (PNV_SPI_MASKED_OPCODE(s->seq_op[((get_seq_index(s) + 1) &
> +                                                0x7)]) == SEQ_OP_SHIFT_N2) {
>                       send_n1_alone = false;
>                   }
>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,

Re: [PATCH v1 2/2] Fixes: Coverity CID 1558831

[Philippe Mathieu-Daudé]

On 6/8/24 15:48, Chalapathi V wrote:
> In this commit the following coverity scan defect has been fixed
> CID 1558831:  Resource leaks  (RESOURCE_LEAK)
>    Variable "rsp_payload" going out of scope leaks the storage it
>    points to.
> 
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>   hw/ssi/pnv_spi.c | 1 +
>   1 file changed, 1 insertion(+)

Fixes: b4cb930e40 ("hw/ssi: Extend SPI model")

Re: [PATCH v1 2/2] Fixes: Coverity CID 1558831

[Philippe Mathieu-Daudé]

Back at this patch since Cédric asked me to look at it.

On 6/8/24 15:48, Chalapathi V wrote:
> In this commit the following coverity scan defect has been fixed
> CID 1558831:  Resource leaks  (RESOURCE_LEAK)
>    Variable "rsp_payload" going out of scope leaks the storage it
>    points to.
> 
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>   hw/ssi/pnv_spi.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index a33f682897..dbe06df224 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -237,6 +237,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>       }
>       if (rsp_payload != NULL) {
>           spi_response(s, s->N1_bits, rsp_payload);
> +        pnv_spi_xfer_buffer_free(rsp_payload);
>       }
>   }
>   

Or bigger patch but simpler logic:

-- >8 --
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index c1297ab733..aaedba84af 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -217,6 +217,9 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
      PnvXferBuffer *rsp_payload = NULL;

      rsp_payload = pnv_spi_xfer_buffer_new();
+    if (!rsp_payload) {
+        return;
+    }
      for (int offset = 0; offset < payload->len; offset += 
s->transfer_len) {
          tx = 0;
          for (int i = 0; i < s->transfer_len; i++) {
@@ -235,9 +238,8 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
                      (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
          }
      }
-    if (rsp_payload != NULL) {
-        spi_response(s, s->N1_bits, rsp_payload);
-    }
+    spi_response(s, s->N1_bits, rsp_payload);
+    pnv_spi_xfer_buffer_free(rsp_payload);
  }

---

I note few things is odd here:

1/ pnv_spi_xfer_buffer_new() uses the GLib g_malloc0() call
    while pnv_spi_xfer_buffer_free() uses plan free().

2/ pnv_spi_xfer_buffer_free() frees payload->data so doesn't
    match pnv_spi_xfer_buffer_new().

This is a bit disappointing.

Re: [PATCH v1 1/2] Fixes: Coverity CID 1558827

[Peter Maydell]

On Tue, 6 Aug 2024 at 14:50, Chalapathi V <chalapathi.v@linux.ibm.com> wrote:

The Subject email for a patch should say what it does
in terms of code changes, not just list the bug number
or coverity issue number, and it should start with the
prefix showing what part of the codebase it is changing.
You can look through other commit messages with "git log"
to see the general style.

> In this commit the following coverity scan defect has been fixed.
> CID 1558827:    (OVERRUN)
>   Overrunning array "s->seq_op" of 8 bytes at byte offset 16
> using index "get_seq_index(s) + 1" (which evaluates to 16).
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  hw/ssi/pnv_spi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index c1297ab733..a33f682897 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -729,7 +729,7 @@ static void operation_sequencer(PnvSpi *s)
>       * some operations may cause more than one frame to be sequenced.
>       */
>      while (get_seq_index(s) < NUM_SEQ_OPS) {
> -        opcode = s->seq_op[get_seq_index(s)];
> +        opcode = s->seq_op[(get_seq_index(s) & 0x7)];

This doesn't seem like the right fix, as Philippe points out.
It's also not any of the possible approaches I suggested
in my email in the other thread.

 * if we're confident that this value really can't be more than
   7, then we should assert() that
 * if the value might be more than 7 if the guest has done something
   silly, we should arrange to detect and handle that error
 * if the hardware really ignores the high bit of the field,
   that should be implemented in get_seq_index(), not in its callers
 * we should consider whether using a local variable instead
   of repeatedly calling get_seq_index() might make the code
   easier to read (as well as helping Coverity)

>          /* Set sequencer state to decode */
>          s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
>          /*
> @@ -834,8 +834,8 @@ static void operation_sequencer(PnvSpi *s)
>                   * transmission to the responder without requiring a refill of
>                   * the TDR between the two operations.
>                   */
> -                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
> -                                == SEQ_OP_SHIFT_N2) {
> +                if (PNV_SPI_MASKED_OPCODE(s->seq_op[((get_seq_index(s) + 1) &
> +                                                0x7)]) == SEQ_OP_SHIFT_N2) {

This doesn't look right. If operation 7 is SHIFT_N1 then we
do not want to look at operation 0 (which is what
"(get_seq_index(s) + 1) & 0x7" will cause us to look at), because
operation 0 is unrelated. What we want to do is have this condition
be "if (sequence index != 7 && s->seq_op[sequence index + 1] is SHIFT_N2)".

>                      send_n1_alone = false;
>                  }
>                  s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,

thanks
-- PMM

Re: [PATCH v1 2/2] Fixes: Coverity CID 1558831

[Peter Maydell]

On Wed, 7 Aug 2024 at 21:13, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Back at this patch since Cédric asked me to look at it.
>
> On 6/8/24 15:48, Chalapathi V wrote:
> > In this commit the following coverity scan defect has been fixed
> > CID 1558831:  Resource leaks  (RESOURCE_LEAK)
> >    Variable "rsp_payload" going out of scope leaks the storage it
> >    points to.
> >
> > Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> > ---
> >   hw/ssi/pnv_spi.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> > index a33f682897..dbe06df224 100644
> > --- a/hw/ssi/pnv_spi.c
> > +++ b/hw/ssi/pnv_spi.c
> > @@ -237,6 +237,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
> >       }
> >       if (rsp_payload != NULL) {
> >           spi_response(s, s->N1_bits, rsp_payload);
> > +        pnv_spi_xfer_buffer_free(rsp_payload);
> >       }
> >   }
> >
>
> Or bigger patch but simpler logic:
>
> -- >8 --
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index c1297ab733..aaedba84af 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -217,6 +217,9 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>       PnvXferBuffer *rsp_payload = NULL;
>
>       rsp_payload = pnv_spi_xfer_buffer_new();
> +    if (!rsp_payload) {
> +        return;
> +    }

pnv_spi_xfer_buffer_new() cannot fail (it calls
g_malloc0, which aborts on not having enough memory)
so we don't need to check it for NULL.

>       for (int offset = 0; offset < payload->len; offset +=
> s->transfer_len) {
>           tx = 0;
>           for (int i = 0; i < s->transfer_len; i++) {
> @@ -235,9 +238,8 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>                       (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>           }
>       }
> -    if (rsp_payload != NULL) {
> -        spi_response(s, s->N1_bits, rsp_payload);
> -    }

(So this NULL check was unnecessary in the old code.)

> +    spi_response(s, s->N1_bits, rsp_payload);
> +    pnv_spi_xfer_buffer_free(rsp_payload);
>   }
>
> ---
>
> I note few things is odd here:
>
> 1/ pnv_spi_xfer_buffer_new() uses the GLib g_malloc0() call
>     while pnv_spi_xfer_buffer_free() uses plan free().

This does work (glib guarantees now that g_malloc and friends
can be paired with free), but it is not great style.

> 2/ pnv_spi_xfer_buffer_free() frees payload->data so doesn't
>     match pnv_spi_xfer_buffer_new().

This part I think is OK -- the payload->data buffer is
allocated with g_realloc(), so pnv_spi_xfer_buffer_new()
creates a valid initial state (payload->data = NULL and
payload->len = 0), pnv_spi_xfer_buffer_write_ptr() may
allocate or extend the buffer (using g_realloc and
updating the payload->len to match), and
pnv_spi_xfer_buffer_free() will free the buffer
(including handling the "buffer never allocated so
payload->data = NULL" case, because free(NULL) is OK).

What it doesn't get right I think is that when
pnv_spi_xfer_buffer_write_ptr() extends the payload->data
buffer it doesn't zero the newly added memory, so I think
we might end up giving the guest back the contents of
uninitialized memory.

Another style issue is:
    PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
g_new0(PnvXferBuffer, 1) is the better way to say
"allocate me a zeroed out PnvXferBuffer", rather than
manual use of sizeof.

The various places which do
    if (*payload == NULL) {
        *payload = pnv_spi_xfer_buffer_new();
    }
also look odd to me -- I'm pretty sure that
in operation_shiftn1() and operation_shiftn2() it
is impossible for them to be called with a NULL payload.

The PnvXferBuffer struct is only 12 bytes large
(a length and a pointer), so it seems rather overkill
to be allocating and freeing it -- is it possible to
have it be a local variable instead?

My other question here is whether we need to be doing
dynamic memory allocation, extension and freeing of the
payload->data at all. This is quite an unusual thing to
need to do in a hardware device model, because usually
the real hardware we're modelling doesn't have infinite
resources, it has some fixed amount of memory that it
has to work with. If the guest can effectively ask
us to allocate arbitrary amounts of memory, do we
have appropriate guards in place to forbid that?

Is there a datasheet for this device?

thanks
-- PMM