QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c
@ 2020-07-27 17:08 Mauro Matteo Cascella
  2020-07-27 17:08 ` [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags Mauro Matteo Cascella
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Mauro Matteo Cascella @ 2020-07-27 17:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: jasowang, dmitry.fleytman, mcascell, ezrakiez

An assertion failure issue was reported by Mr. Ziming Zhang (CC'd).
It occurs in the code that processes network packets while adding data
fragments into packet context. This flaw could potentially be abused by
a malicious guest to abort the QEMU process on the host. This two patch
series does a couple of things:

- introduces a new function in net_tx_pkt.{c,h} to check the maximum number
  of data fragments
- adds a check in both e1000e and vmxnet3 devices to skip the packet if the
  current data fragment exceeds max_raw_frags, preventing
  net_tx_pkt_add_raw_fragment() to be called with an invalid raw_frags

Mauro Matteo Cascella (2):
  hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  hw/net: check max_raw_frags in e1000e and vmxnet3 devices

 hw/net/e1000e_core.c | 3 ++-
 hw/net/net_tx_pkt.c  | 5 +++++
 hw/net/net_tx_pkt.h  | 8 ++++++++
 hw/net/vmxnet3.c     | 3 ++-
 4 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  2020-07-27 17:08 [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Mauro Matteo Cascella
@ 2020-07-27 17:08 ` Mauro Matteo Cascella
  2020-07-28  4:06   ` Jason Wang
  2020-07-27 17:08 ` [PATCH 2/2] hw/net: check max_raw_frags in e1000e and vmxnet3 devices Mauro Matteo Cascella
  2020-07-27 17:29 ` [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Alexander Bulekov
  2 siblings, 1 reply; 11+ messages in thread
From: Mauro Matteo Cascella @ 2020-07-27 17:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: jasowang, dmitry.fleytman, mcascell, ezrakiez

This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
current data fragment against the maximum number of data fragments.

Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
---
 hw/net/net_tx_pkt.c | 5 +++++
 hw/net/net_tx_pkt.h | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index 9560e4a49e..d035618f2c 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
     }
 }
 
+bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
+{
+    return pkt->raw_frags >= pkt->max_raw_frags;
+}
+
 bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
 {
     return pkt->raw_frags > 0;
diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
index 4ec8bbe9bd..e2ee46ae03 100644
--- a/hw/net/net_tx_pkt.h
+++ b/hw/net/net_tx_pkt.h
@@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
  */
 bool net_tx_pkt_parse(struct NetTxPkt *pkt);
 
+/**
+* indicates if the current data fragment exceeds max_raw_frags
+*
+* @pkt:            packet
+*
+*/
+bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
+
 /**
 * indicates if there are data fragments held by this packet object.
 *
-- 
2.26.2



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] hw/net: check max_raw_frags in e1000e and vmxnet3 devices
  2020-07-27 17:08 [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Mauro Matteo Cascella
  2020-07-27 17:08 ` [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags Mauro Matteo Cascella
@ 2020-07-27 17:08 ` Mauro Matteo Cascella
  2020-07-27 17:29 ` [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Alexander Bulekov
  2 siblings, 0 replies; 11+ messages in thread
From: Mauro Matteo Cascella @ 2020-07-27 17:08 UTC (permalink / raw)
  To: qemu-devel; +Cc: jasowang, dmitry.fleytman, mcascell, ezrakiez

This patch adds a check in both e1000e and vmxnet3 devices to skip the packet
if the current data fragment exceeds max_raw_frags, preventing
net_tx_pkt_add_raw_fragment() to be called with an invalid raw_frags.

Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
---
 hw/net/e1000e_core.c | 3 ++-
 hw/net/vmxnet3.c     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index bcd186cac5..c573a30d63 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -728,7 +728,8 @@ e1000e_process_tx_desc(E1000ECore *core,
     addr = le64_to_cpu(dp->buffer_addr);
 
     if (!tx->skip_cp) {
-        if (!net_tx_pkt_add_raw_fragment(tx->tx_pkt, addr, split_size)) {
+        if (net_tx_pkt_exceed_max_fragments(tx->tx_pkt) ||
+            !net_tx_pkt_add_raw_fragment(tx->tx_pkt, addr, split_size)) {
             tx->skip_cp = true;
         }
     }
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 7a6ca4ec35..f482806037 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -650,7 +650,8 @@ static void vmxnet3_process_tx_queue(VMXNET3State *s, int qidx)
             data_len = (txd.len > 0) ? txd.len : VMXNET3_MAX_TX_BUF_SIZE;
             data_pa = txd.addr;
 
-            if (!net_tx_pkt_add_raw_fragment(s->tx_pkt,
+            if (net_tx_pkt_exceed_max_fragments(s->tx_pkt) ||
+                !net_tx_pkt_add_raw_fragment(s->tx_pkt,
                                                 data_pa,
                                                 data_len)) {
                 s->skip_current_tx_pkt = true;
-- 
2.26.2



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c
  2020-07-27 17:08 [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Mauro Matteo Cascella
  2020-07-27 17:08 ` [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags Mauro Matteo Cascella
  2020-07-27 17:08 ` [PATCH 2/2] hw/net: check max_raw_frags in e1000e and vmxnet3 devices Mauro Matteo Cascella
@ 2020-07-27 17:29 ` Alexander Bulekov
  2020-07-28 16:59   ` Mauro Matteo Cascella
  2020-07-29  8:48   ` Dmitry Fleytman
  2 siblings, 2 replies; 11+ messages in thread
From: Alexander Bulekov @ 2020-07-27 17:29 UTC (permalink / raw)
  To: Mauro Matteo Cascella; +Cc: jasowang, dmitry.fleytman, qemu-devel, ezrakiez

I sent a reproducer for the to the list some time ago, but never created
a Launchpad bug...
https://www.mail-archive.com/qemu-devel@nongnu.org/msg701930.html

Anyways.. I can confirm that I can't reproduce the issue with these
patches.

Minimized Reproducer:
cat << EOF | ./i386-softmmu/qemu-system-i386 -M pc-q35-5.0 -nographic \
-display none -serial none -monitor none -qtest stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xe1020000
outl 0xcf8 0x80001004
outw 0xcfc 0x7
write 0xe10207e8 0x4 0x25ff13ff
write 0xe10200b8 0x7 0xe3055e411b0202
write 0xe1020100 0x5 0x5e411b0202
write 0xe1020110 0x4 0x1b0202e1
write 0xe1020118 0x4 0x06fff105
write 0xe1020128 0x7 0xf3055e411b0202
write 0xe1020402 0x2 0x5e41
write 0xe1020420 0x4 0x1b0202e1
write 0xe1020428 0x4 0x06ff6105
write 0xe1020438 0x1 0x63
write 0xe1020439 0x1 0x05
EOF

-Alex

On 200727 1908, Mauro Matteo Cascella wrote:
> An assertion failure issue was reported by Mr. Ziming Zhang (CC'd).
> It occurs in the code that processes network packets while adding data
> fragments into packet context. This flaw could potentially be abused by
> a malicious guest to abort the QEMU process on the host. This two patch
> series does a couple of things:
> 
> - introduces a new function in net_tx_pkt.{c,h} to check the maximum number
>   of data fragments
> - adds a check in both e1000e and vmxnet3 devices to skip the packet if the
>   current data fragment exceeds max_raw_frags, preventing
>   net_tx_pkt_add_raw_fragment() to be called with an invalid raw_frags
> 
> Mauro Matteo Cascella (2):
>   hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
>   hw/net: check max_raw_frags in e1000e and vmxnet3 devices
> 
>  hw/net/e1000e_core.c | 3 ++-
>  hw/net/net_tx_pkt.c  | 5 +++++
>  hw/net/net_tx_pkt.h  | 8 ++++++++
>  hw/net/vmxnet3.c     | 3 ++-
>  4 files changed, 17 insertions(+), 2 deletions(-)
> 
> -- 
> 2.26.2
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  2020-07-27 17:08 ` [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags Mauro Matteo Cascella
@ 2020-07-28  4:06   ` Jason Wang
  2020-07-28 16:26     ` Mauro Matteo Cascella
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2020-07-28  4:06 UTC (permalink / raw)
  To: Mauro Matteo Cascella, qemu-devel; +Cc: dmitry.fleytman, ezrakiez


On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
> current data fragment against the maximum number of data fragments.


I wonder whether it's better to do the check in 
net_tx_pkt_add_raw_fragment() and fail there.

Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when 
returning to true, is this a bug?

Thanks


>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
>   hw/net/net_tx_pkt.c | 5 +++++
>   hw/net/net_tx_pkt.h | 8 ++++++++
>   2 files changed, 13 insertions(+)
>
> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> index 9560e4a49e..d035618f2c 100644
> --- a/hw/net/net_tx_pkt.c
> +++ b/hw/net/net_tx_pkt.c
> @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
>       }
>   }
>   
> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
> +{
> +    return pkt->raw_frags >= pkt->max_raw_frags;
> +}
> +
>   bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
>   {
>       return pkt->raw_frags > 0;
> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> index 4ec8bbe9bd..e2ee46ae03 100644
> --- a/hw/net/net_tx_pkt.h
> +++ b/hw/net/net_tx_pkt.h
> @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
>    */
>   bool net_tx_pkt_parse(struct NetTxPkt *pkt);
>   
> +/**
> +* indicates if the current data fragment exceeds max_raw_frags
> +*
> +* @pkt:            packet
> +*
> +*/
> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
> +
>   /**
>   * indicates if there are data fragments held by this packet object.
>   *



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  2020-07-28  4:06   ` Jason Wang
@ 2020-07-28 16:26     ` Mauro Matteo Cascella
  2020-07-30  5:27       ` Jason Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Mauro Matteo Cascella @ 2020-07-28 16:26 UTC (permalink / raw)
  To: Jason Wang; +Cc: Dmitry Fleytman, QEMU Developers, ziming zhang

On Tue, Jul 28, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
> > This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
> > current data fragment against the maximum number of data fragments.
>
>
> I wonder whether it's better to do the check in
> net_tx_pkt_add_raw_fragment() and fail there.

Given the assertion, I assumed the caller is responsible for the
check, but moving the check in net_tx_pkt_add_raw_fragment() totally
makes sense to me.

> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
> returning to true, is this a bug?

Isn't it unmapped in net_tx_pkt_reset()?

> Thanks
>
>
> >
> > Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >   hw/net/net_tx_pkt.c | 5 +++++
> >   hw/net/net_tx_pkt.h | 8 ++++++++
> >   2 files changed, 13 insertions(+)
> >
> > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> > index 9560e4a49e..d035618f2c 100644
> > --- a/hw/net/net_tx_pkt.c
> > +++ b/hw/net/net_tx_pkt.c
> > @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
> >       }
> >   }
> >
> > +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
> > +{
> > +    return pkt->raw_frags >= pkt->max_raw_frags;
> > +}
> > +
> >   bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
> >   {
> >       return pkt->raw_frags > 0;
> > diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> > index 4ec8bbe9bd..e2ee46ae03 100644
> > --- a/hw/net/net_tx_pkt.h
> > +++ b/hw/net/net_tx_pkt.h
> > @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
> >    */
> >   bool net_tx_pkt_parse(struct NetTxPkt *pkt);
> >
> > +/**
> > +* indicates if the current data fragment exceeds max_raw_frags
> > +*
> > +* @pkt:            packet
> > +*
> > +*/
> > +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
> > +
> >   /**
> >   * indicates if there are data fragments held by this packet object.
> >   *
>

-- 
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c
  2020-07-27 17:29 ` [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Alexander Bulekov
@ 2020-07-28 16:59   ` Mauro Matteo Cascella
  2020-07-29  8:48   ` Dmitry Fleytman
  1 sibling, 0 replies; 11+ messages in thread
From: Mauro Matteo Cascella @ 2020-07-28 16:59 UTC (permalink / raw)
  To: Alexander Bulekov
  Cc: Jason Wang, Dmitry Fleytman, QEMU Developers, ziming zhang

Thank you Alexander for testing the patch and providing the
reproducer. I think you should be credited, along with Ziming, for
independently reporting the same issue.

On Mon, Jul 27, 2020 at 7:40 PM Alexander Bulekov <alxndr@bu.edu> wrote:
>
> I sent a reproducer for the to the list some time ago, but never created
> a Launchpad bug...
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg701930.html
>
> Anyways.. I can confirm that I can't reproduce the issue with these
> patches.
>
> Minimized Reproducer:
> cat << EOF | ./i386-softmmu/qemu-system-i386 -M pc-q35-5.0 -nographic \
> -display none -serial none -monitor none -qtest stdio
> outl 0xcf8 0x80001010
> outl 0xcfc 0xe1020000
> outl 0xcf8 0x80001004
> outw 0xcfc 0x7
> write 0xe10207e8 0x4 0x25ff13ff
> write 0xe10200b8 0x7 0xe3055e411b0202
> write 0xe1020100 0x5 0x5e411b0202
> write 0xe1020110 0x4 0x1b0202e1
> write 0xe1020118 0x4 0x06fff105
> write 0xe1020128 0x7 0xf3055e411b0202
> write 0xe1020402 0x2 0x5e41
> write 0xe1020420 0x4 0x1b0202e1
> write 0xe1020428 0x4 0x06ff6105
> write 0xe1020438 0x1 0x63
> write 0xe1020439 0x1 0x05
> EOF
>
> -Alex
>
> On 200727 1908, Mauro Matteo Cascella wrote:
> > An assertion failure issue was reported by Mr. Ziming Zhang (CC'd).
> > It occurs in the code that processes network packets while adding data
> > fragments into packet context. This flaw could potentially be abused by
> > a malicious guest to abort the QEMU process on the host. This two patch
> > series does a couple of things:
> >
> > - introduces a new function in net_tx_pkt.{c,h} to check the maximum number
> >   of data fragments
> > - adds a check in both e1000e and vmxnet3 devices to skip the packet if the
> >   current data fragment exceeds max_raw_frags, preventing
> >   net_tx_pkt_add_raw_fragment() to be called with an invalid raw_frags
> >
> > Mauro Matteo Cascella (2):
> >   hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
> >   hw/net: check max_raw_frags in e1000e and vmxnet3 devices
> >
> >  hw/net/e1000e_core.c | 3 ++-
> >  hw/net/net_tx_pkt.c  | 5 +++++
> >  hw/net/net_tx_pkt.h  | 8 ++++++++
> >  hw/net/vmxnet3.c     | 3 ++-
> >  4 files changed, 17 insertions(+), 2 deletions(-)
> >
> > --
> > 2.26.2
> >
> >
>


-- 
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c
  2020-07-27 17:29 ` [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Alexander Bulekov
  2020-07-28 16:59   ` Mauro Matteo Cascella
@ 2020-07-29  8:48   ` Dmitry Fleytman
  1 sibling, 0 replies; 11+ messages in thread
From: Dmitry Fleytman @ 2020-07-29  8:48 UTC (permalink / raw)
  To: Alexander Bulekov; +Cc: Jason Wang, Mauro Matteo Cascella, qemu-devel, ezrakiez

Reviewed-by: Dmitry Fleytman <dmitry.fleytman@gmail.com>

The idea looks good to me. I believe it makes sense to do the check in net_tx_pkt_add_raw_fragment() as suggested by Jason.

> On 27 Jul 2020, at 20:29, Alexander Bulekov <alxndr@bu.edu> wrote:
> 
> I sent a reproducer for the to the list some time ago, but never created
> a Launchpad bug...
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg701930.html
> 
> Anyways.. I can confirm that I can't reproduce the issue with these
> patches.
> 
> Minimized Reproducer:
> cat << EOF | ./i386-softmmu/qemu-system-i386 -M pc-q35-5.0 -nographic \
> -display none -serial none -monitor none -qtest stdio
> outl 0xcf8 0x80001010
> outl 0xcfc 0xe1020000
> outl 0xcf8 0x80001004
> outw 0xcfc 0x7
> write 0xe10207e8 0x4 0x25ff13ff
> write 0xe10200b8 0x7 0xe3055e411b0202
> write 0xe1020100 0x5 0x5e411b0202
> write 0xe1020110 0x4 0x1b0202e1
> write 0xe1020118 0x4 0x06fff105
> write 0xe1020128 0x7 0xf3055e411b0202
> write 0xe1020402 0x2 0x5e41
> write 0xe1020420 0x4 0x1b0202e1
> write 0xe1020428 0x4 0x06ff6105
> write 0xe1020438 0x1 0x63
> write 0xe1020439 0x1 0x05
> EOF
> 
> -Alex
> 
> On 200727 1908, Mauro Matteo Cascella wrote:
>> An assertion failure issue was reported by Mr. Ziming Zhang (CC'd).
>> It occurs in the code that processes network packets while adding data
>> fragments into packet context. This flaw could potentially be abused by
>> a malicious guest to abort the QEMU process on the host. This two patch
>> series does a couple of things:
>> 
>> - introduces a new function in net_tx_pkt.{c,h} to check the maximum number
>>  of data fragments
>> - adds a check in both e1000e and vmxnet3 devices to skip the packet if the
>>  current data fragment exceeds max_raw_frags, preventing
>>  net_tx_pkt_add_raw_fragment() to be called with an invalid raw_frags
>> 
>> Mauro Matteo Cascella (2):
>>  hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
>>  hw/net: check max_raw_frags in e1000e and vmxnet3 devices
>> 
>> hw/net/e1000e_core.c | 3 ++-
>> hw/net/net_tx_pkt.c  | 5 +++++
>> hw/net/net_tx_pkt.h  | 8 ++++++++
>> hw/net/vmxnet3.c     | 3 ++-
>> 4 files changed, 17 insertions(+), 2 deletions(-)
>> 
>> -- 
>> 2.26.2
>> 
>> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  2020-07-28 16:26     ` Mauro Matteo Cascella
@ 2020-07-30  5:27       ` Jason Wang
  2020-07-30 17:05         ` Mauro Matteo Cascella
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2020-07-30  5:27 UTC (permalink / raw)
  To: Mauro Matteo Cascella; +Cc: Dmitry Fleytman, QEMU Developers, ziming zhang


On 2020/7/29 上午12:26, Mauro Matteo Cascella wrote:
> On Tue, Jul 28, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
>>> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
>>> current data fragment against the maximum number of data fragments.
>>
>> I wonder whether it's better to do the check in
>> net_tx_pkt_add_raw_fragment() and fail there.
> Given the assertion, I assumed the caller is responsible for the
> check, but moving the check in net_tx_pkt_add_raw_fragment() totally
> makes sense to me.


Want to send a new version for this?


>
>> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
>> returning to true, is this a bug?
> Isn't it unmapped in net_tx_pkt_reset()?


Probably but see how it was used in e1000e, the net_tx_pkt_reset() is 
only called when eop is set. Is this a bug?

Thanks

>
>> Thanks
>>
>>
>>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>> ---
>>>    hw/net/net_tx_pkt.c | 5 +++++
>>>    hw/net/net_tx_pkt.h | 8 ++++++++
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>>> index 9560e4a49e..d035618f2c 100644
>>> --- a/hw/net/net_tx_pkt.c
>>> +++ b/hw/net/net_tx_pkt.c
>>> @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
>>>        }
>>>    }
>>>
>>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
>>> +{
>>> +    return pkt->raw_frags >= pkt->max_raw_frags;
>>> +}
>>> +
>>>    bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
>>>    {
>>>        return pkt->raw_frags > 0;
>>> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
>>> index 4ec8bbe9bd..e2ee46ae03 100644
>>> --- a/hw/net/net_tx_pkt.h
>>> +++ b/hw/net/net_tx_pkt.h
>>> @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
>>>     */
>>>    bool net_tx_pkt_parse(struct NetTxPkt *pkt);
>>>
>>> +/**
>>> +* indicates if the current data fragment exceeds max_raw_frags
>>> +*
>>> +* @pkt:            packet
>>> +*
>>> +*/
>>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
>>> +
>>>    /**
>>>    * indicates if there are data fragments held by this packet object.
>>>    *



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  2020-07-30  5:27       ` Jason Wang
@ 2020-07-30 17:05         ` Mauro Matteo Cascella
  2020-07-31  3:33           ` Jason Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Mauro Matteo Cascella @ 2020-07-30 17:05 UTC (permalink / raw)
  To: Jason Wang; +Cc: Dmitry Fleytman, QEMU Developers, ziming zhang

On Thu, Jul 30, 2020 at 7:28 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/7/29 上午12:26, Mauro Matteo Cascella wrote:
> > On Tue, Jul 28, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
> >>> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
> >>> current data fragment against the maximum number of data fragments.
> >>
> >> I wonder whether it's better to do the check in
> >> net_tx_pkt_add_raw_fragment() and fail there.
> > Given the assertion, I assumed the caller is responsible for the
> > check, but moving the check in net_tx_pkt_add_raw_fragment() totally
> > makes sense to me.
>
>
> Want to send a new version for this?

Sure, I'll send a new version. Thank you.

>
> >
> >> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
> >> returning to true, is this a bug?
> > Isn't it unmapped in net_tx_pkt_reset()?
>
>
> Probably but see how it was used in e1000e, the net_tx_pkt_reset() is
> only called when eop is set. Is this a bug?

Yeah it all depends on E1000_TXD_CMD_EOP. Besides, if not set,
e1000e_tx_pkt_send() would never be called. Honestly, I don't know if
this is a reasonable scenario or not.

> Thanks
>
> >
> >> Thanks
> >>
> >>
> >>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> >>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> >>> ---
> >>>    hw/net/net_tx_pkt.c | 5 +++++
> >>>    hw/net/net_tx_pkt.h | 8 ++++++++
> >>>    2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> >>> index 9560e4a49e..d035618f2c 100644
> >>> --- a/hw/net/net_tx_pkt.c
> >>> +++ b/hw/net/net_tx_pkt.c
> >>> @@ -400,6 +400,11 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
> >>>        }
> >>>    }
> >>>
> >>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt)
> >>> +{
> >>> +    return pkt->raw_frags >= pkt->max_raw_frags;
> >>> +}
> >>> +
> >>>    bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
> >>>    {
> >>>        return pkt->raw_frags > 0;
> >>> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> >>> index 4ec8bbe9bd..e2ee46ae03 100644
> >>> --- a/hw/net/net_tx_pkt.h
> >>> +++ b/hw/net/net_tx_pkt.h
> >>> @@ -179,6 +179,14 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc);
> >>>     */
> >>>    bool net_tx_pkt_parse(struct NetTxPkt *pkt);
> >>>
> >>> +/**
> >>> +* indicates if the current data fragment exceeds max_raw_frags
> >>> +*
> >>> +* @pkt:            packet
> >>> +*
> >>> +*/
> >>> +bool net_tx_pkt_exceed_max_fragments(struct NetTxPkt *pkt);
> >>> +
> >>>    /**
> >>>    * indicates if there are data fragments held by this packet object.
> >>>    *
>

--
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags
  2020-07-30 17:05         ` Mauro Matteo Cascella
@ 2020-07-31  3:33           ` Jason Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Jason Wang @ 2020-07-31  3:33 UTC (permalink / raw)
  To: Mauro Matteo Cascella; +Cc: Dmitry Fleytman, QEMU Developers, ziming zhang


On 2020/7/31 上午1:05, Mauro Matteo Cascella wrote:
> On Thu, Jul 30, 2020 at 7:28 AM Jason Wang<jasowang@redhat.com>  wrote:
>> On 2020/7/29 上午12:26, Mauro Matteo Cascella wrote:
>>> On Tue, Jul 28, 2020 at 6:06 AM Jason Wang<jasowang@redhat.com>  wrote:
>>>> On 2020/7/28 上午1:08, Mauro Matteo Cascella wrote:
>>>>> This patch introduces a new function in hw/net/net_tx_pkt.{c,h} to check the
>>>>> current data fragment against the maximum number of data fragments.
>>>> I wonder whether it's better to do the check in
>>>> net_tx_pkt_add_raw_fragment() and fail there.
>>> Given the assertion, I assumed the caller is responsible for the
>>> check, but moving the check in net_tx_pkt_add_raw_fragment() totally
>>> makes sense to me.
>> Want to send a new version for this?
> Sure, I'll send a new version. Thank you.
>
>>>> Btw, I find net_tx_pkt_add_raw_fragment() does not unmap dma when
>>>> returning to true, is this a bug?
>>> Isn't it unmapped in net_tx_pkt_reset()?
>> Probably but see how it was used in e1000e, the net_tx_pkt_reset() is
>> only called when eop is set. Is this a bug?
> Yeah it all depends on E1000_TXD_CMD_EOP. Besides, if not set,
> e1000e_tx_pkt_send() would never be called. Honestly, I don't know if
> this is a reasonable scenario or not.


It's probably fine since anyway e1000e_core_reset() will call 
net_tx_pkt_reset().

Thanks


>
>> Thanks
>>



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27 17:08 [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Mauro Matteo Cascella
2020-07-27 17:08 ` [PATCH 1/2] hw/net/net_tx_pkt: add function to check pkt->max_raw_frags Mauro Matteo Cascella
2020-07-28  4:06   ` Jason Wang
2020-07-28 16:26     ` Mauro Matteo Cascella
2020-07-30  5:27       ` Jason Wang
2020-07-30 17:05         ` Mauro Matteo Cascella
2020-07-31  3:33           ` Jason Wang
2020-07-27 17:08 ` [PATCH 2/2] hw/net: check max_raw_frags in e1000e and vmxnet3 devices Mauro Matteo Cascella
2020-07-27 17:29 ` [PATCH 0/2] assertion failure in net_tx_pkt_add_raw_fragment() in hw/net/net_tx_pkt.c Alexander Bulekov
2020-07-28 16:59   ` Mauro Matteo Cascella
2020-07-29  8:48   ` Dmitry Fleytman

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git