On Mon, 29 Jul 2024 at 11:33, Cédric Le Goater <c...@kaod.org> wrote:
>
> On 7/26/24 01:53, Nicholas Piggin wrote:
> > From: Chalapathi V <chalapath...@linux.ibm.com>
> >
> > In this commit SPI shift engine and sequencer logic is implemented.
> > Shift engine performs serialization and de-serialization according to the
> > control by the sequencer and according to the setup defined in the
> > configuration registers. Sequencer implements the main control logic and
> > FSM to handle data transmit and data receive control of the shift engine.
> > +static inline uint8_t get_seq_index(PnvSpi *s)
> > +{
> > + return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
> > +}
>
> Coverity reports :
>
> >>> 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).
>
>
> get_seq_index() can return a value between 0 and 15 and it is used in a couple
> of places to index array s->seq_op[] which is an 8 bytes array.
>
> Should we increase the size of the seq_op array ?
I think in most places this can't happen, because:
> > + /*
> > + * There are only 8 possible operation IDs to iterate through though
> > + * some operations may cause more than one frame to be sequenced.
> > + */
> > + while (get_seq_index(s) < NUM_SEQ_OPS) {
The loop condition enforces that the sequence index is < 8.
> > + opcode = s->seq_op[get_seq_index(s)];
Coverity isn't smart enough to be able to tell that this
call to get_seq_index() must return the same value as the
previous one and so be in bounds for the array, which is why it's
complaining.
On the other hand:
* this is also not totally obvious to humans
* it means we're doing the shift-and-mask ops to get the
sequence index out every time
Maybe we should keep it in a variable? (Depends whether there's
a bunch of places in the loop that want to update the index, or
if we just do that at the end of the loop.)
Another option would be to have a version of get_seq_index() that
asserts that the index is valid.
The other design approach available here is to have the device
state struct hold the underlying information in a split-out way
(so e.g s->seq_index and other fields), and then assemble those
into a 32-bit value for the status register in the "guest reads/writes
the register codepath).
That said, there is also a case which I suspect really is an
overrun bug:
> > + case SEQ_OP_SHIFT_N1:
> > + s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
> > SEQ_STATE_EXECUTE);
> > + trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
> > + /*
> > + * Only allow a shift_n1 when the state is not IDLE or DONE.
> > + * In either of those two cases the sequencer is not in a
> > proper
> > + * state to perform shift operations because the sequencer has:
> > + * - processed a responder deselect (DONE)
> > + * - processed a stop opcode (IDLE)
> > + * - encountered an error (IDLE)
> > + */
> > + if ((GETFIELD(SPI_STS_SHIFTER_FSM, s->status) == FSM_IDLE) ||
> > + (GETFIELD(SPI_STS_SHIFTER_FSM, s->status) == FSM_DONE)) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "Shift_N1 not allowed in "
> > + "shifter state = 0x%llx", GETFIELD(
> > + SPI_STS_SHIFTER_FSM, s->status));
> > + /*
> > + * Set sequencer FSM error bit 3 (general_SPI_status[3])
> > + * in status reg.
> > + */
> > + s->status = SETFIELD(SPI_STS_GEN_STATUS_B3, s->status, 1);
> > + trace_pnv_spi_sequencer_stop_requested("invalid shifter
> > state");
> > + stop = true;
> > + } else {
> > + /*
> > + * Look for the special case where there is a shift_n1 set
> > for
> > + * transmit and it is followed by a shift_n2 set for
> > transmit
> > + * AND the combined transmit length of the two operations
> > is
> > + * less than or equal to the size of the TDR register. In
> > this
> > + * case we want to use both this current shift_n1 opcode
> > and the
> > + * following shift_n2 opcode to assemble the frame for
> > + * 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) {
Here we look at the seq_op[] array for get_seq_index(s) + 1,
so if we're on the last iteration of the loop because the seq
index is 7 we'll read off the end of the array at index 8.
(Presumably the correct behaviour if the shift_n1 is the last
operation is that it's not this special case.)
> > + send_n1_alone = false;
> > + }
> > + s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> > + FSM_SHIFT_N1);
thanks
-- PMM
