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,