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.1On 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
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,
© 2016 - 2024 Red Hat, Inc.