diff mbox series

hw/net: Fix Coverity Issue for npcm-gmac

Message ID 20240618172527.3450098-1-nabihestefan@google.com (mailing list archive)
State New, archived
Headers show
Series hw/net: Fix Coverity Issue for npcm-gmac | expand

Commit Message

Nabih Estefan June 18, 2024, 5:25 p.m. UTC
There is an extra `buf=` set that is not used by npcm-gmac. Remove it
for coverity to be happy.

Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 hw/net/npcm_gmac.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Alex Bennée June 19, 2024, 9:13 a.m. UTC | #1
Nabih Estefan <nabihestefan@google.com> writes:

> There is an extra `buf=` set that is not used by npcm-gmac. Remove it
> for coverity to be happy.

Have you go the coverity reference to include in the commit message?

>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>  hw/net/npcm_gmac.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
> index 1b71e2526e..b397fd5064 100644
> --- a/hw/net/npcm_gmac.c
> +++ b/hw/net/npcm_gmac.c
> @@ -614,7 +614,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
>              net_checksum_calculate(tx_send_buffer, length, csum);
>              qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length);
>              trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length);
> -            buf = tx_send_buffer;

So coverity is saying that buf starts at tx_send_buffer and none of the
other legs that can mess with it are possible for the tx_desc.tdes1 &
TX_DESC_TDES1_LAST_SEG_MASK leg?

Or that buf should always start at tx_send_buffer and only ever advance
if we grow the size of the tx_send_buffer?


>              length = 0;
>          }
Peter Maydell June 20, 2024, 9:52 a.m. UTC | #2
On Wed, 19 Jun 2024 at 10:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Nabih Estefan <nabihestefan@google.com> writes:
>
> > There is an extra `buf=` set that is not used by npcm-gmac. Remove it
> > for coverity to be happy.

By the way, Nabih, it looks like the mailing list received five copies
of this patch email. You might want to look at what happened on your
end that resulted in all the duplicates.

> Have you go the coverity reference to include in the commit message?

This is CID 1534027.

> > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > ---
> >  hw/net/npcm_gmac.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
> > index 1b71e2526e..b397fd5064 100644
> > --- a/hw/net/npcm_gmac.c
> > +++ b/hw/net/npcm_gmac.c
> > @@ -614,7 +614,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
> >              net_checksum_calculate(tx_send_buffer, length, csum);
> >              qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length);
> >              trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length);
> > -            buf = tx_send_buffer;
>
> So coverity is saying that buf starts at tx_send_buffer and none of the
> other legs that can mess with it are possible for the tx_desc.tdes1 &
> TX_DESC_TDES1_LAST_SEG_MASK leg?

Coverity is saying that in the loop body, we unconditionally
(in "step 4") set "buf = &tx_send_buffer[prev_buf_size]" before
we ever try to use "buf". This assignment "buf = tx_send_buffer"
happens later in the loop body, but there is no further reference
to buf either inside the loop body or after the loop ends. So we
will never look at the value we assign to "buf" here (either we
finish the loop and the function, or else we loop back around
again and overwrite this value), and this assignment is dead code.

What I'm wondering is whether this code for "last segment,
send the packet" should be setting "prev_buf_size = 0" instead
of "buf = tx_send_buffer" (meaning, I think "we've sent this packet,
there is nothing currently in the tx_send_buffer, the next descriptor
can start filling tx_send_buffer from byte 0".) Otherwise I think
we will continue to accumulate data from the following descriptor
into tx_send_buffer after the data from this packet, but when we
send that second packet we will do it from the start of
tx_send_buffer and so we will send the wrong data.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
index 1b71e2526e..b397fd5064 100644
--- a/hw/net/npcm_gmac.c
+++ b/hw/net/npcm_gmac.c
@@ -614,7 +614,6 @@  static void gmac_try_send_next_packet(NPCMGMACState *gmac)
             net_checksum_calculate(tx_send_buffer, length, csum);
             qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length);
             trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length);
-            buf = tx_send_buffer;
             length = 0;
         }