[PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Nabih Estefan]

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(-)

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;
         }
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH] hw/net: Fix Coverity Issue for nocm-gmac

[Nabih Estefan]

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(-)

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;
         }
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Nabih Estefan]

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(-)

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;
         }
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Nabih Estefan]

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(-)

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;
         }
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Nabih Estefan]

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(-)

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;
         }
 
-- 
2.45.2.627.g7a2c4fd464-goog

RE: [PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Zhang, Chen]

> -----Original Message-----
> From: qemu-devel-bounces+chen.zhang=intel.com@nongnu.org <qemu-
> devel-bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Nabih
> Estefan
> Sent: Wednesday, June 19, 2024 1:25 AM
> To: peter.maydell@linaro.org
> Cc: qemu-devel@nongnu.org; kfting@nuvoton.com; wuhaotsh@google.com;
> jasowang@redhat.org; nabihestefan@google.com
> Subject: [PATCH] hw/net: Fix Coverity Issue for npcm-gmac
> 
> There is an extra `buf=` set that is not used by npcm-gmac. Remove it for
> coverity to be happy.
> 

Looks good to me.
By the way, I see this function comments the Steps 1/2/3...
But I can't find the page 384 of datasheet, please add more details about the datasheet(versions of datasheet, URL....).

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> 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;
>              length = 0;
>          }
> 
> --
> 2.45.2.627.g7a2c4fd464-goog
> 

Re: [PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Alex Bennée]

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;
>          }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Peter Maydell]

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