diff mbox series

net: e1000: check transmit descriptor field values

Message ID 20210210145258.143131-1-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series net: e1000: check transmit descriptor field values | expand

Commit Message

Prasad Pandit Feb. 10, 2021, 2:52 p.m. UTC
From: Prasad J Pandit <pjp@fedoraproject.org>

While processing transmit (tx) descriptors in process_tx_desc()
various descriptor fields are not checked properly. This may lead
to infinite loop like issue. Add checks to avoid them.

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/net/e1000.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jason Wang Feb. 18, 2021, 5:53 a.m. UTC | #1
On 2021/2/10 下午10:52, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While processing transmit (tx) descriptors in process_tx_desc()
> various descriptor fields are not checked properly. This may lead
> to infinite loop like issue. Add checks to avoid them.
>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
> Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/net/e1000.c | 6 ++++++
>   1 file changed, 6 insertions(+)


I guess you post the wrong patch :) ?

Thanks


>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index d8da2f6528..15949a3d64 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -667,9 +667,11 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
>   
>       addr = le64_to_cpu(dp->buffer_addr);
>       if (tp->cptse) {
> +        assert(tp->tso_props.hdr_len);
>           msh = tp->tso_props.hdr_len + tp->tso_props.mss;
>           do {
>               bytes = split_size;
> +            assert(msh > tp->size);
>               if (tp->size + bytes > msh)
>                   bytes = msh - tp->size;
>   
> @@ -681,22 +683,26 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
>                   memmove(tp->header, tp->data, tp->tso_props.hdr_len);
>               }
>               tp->size = sz;
> +            assert(tp->size);   /* sz may get truncated */
>               addr += bytes;
>               if (sz == msh) {
>                   xmit_seg(s);
>                   memmove(tp->data, tp->header, tp->tso_props.hdr_len);
>                   tp->size = tp->tso_props.hdr_len;
>               }
> +            assert(split_size >= bytes);
>               split_size -= bytes;
>           } while (bytes && split_size);
>       } else {
>           split_size = MIN(sizeof(tp->data) - tp->size, split_size);
> +        assert(tp->size && split_size);
>           pci_dma_read(d, addr, tp->data + tp->size, split_size);
>           tp->size += split_size;
>       }
>   
>       if (!(txd_lower & E1000_TXD_CMD_EOP))
>           return;
> +    assert(tp->size && tp->tso_props.hdr_len);
>       if (!(tp->cptse && tp->size < tp->tso_props.hdr_len)) {
>           xmit_seg(s);
>       }
Prasad Pandit Feb. 18, 2021, 7:47 a.m. UTC | #2
Hello Jason,

+-- On Thu, 18 Feb 2021, Jason Wang wrote --+
| On 2021/2/10 下午10:52, P J P wrote:
| > From: Prasad J Pandit <pjp@fedoraproject.org>
| >
| > While processing transmit (tx) descriptors in process_tx_desc()
| > various descriptor fields are not checked properly. This may lead
| > to infinite loop like issue. Add checks to avoid them.
| >
| > Reported-by: Alexander Bulekov <alxndr@bu.edu>
| > Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
| > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de>
| > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| > ---
| >   hw/net/e1000.c | 6 ++++++
| >   1 file changed, 6 insertions(+)
|
| I guess you post the wrong patch :) ?

Wrong patch...?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Jason Wang Feb. 19, 2021, 2:53 a.m. UTC | #3
On 2021/2/18 3:47 下午, P J P wrote:
>    Hello Jason,
>
> +-- On Thu, 18 Feb 2021, Jason Wang wrote --+
> | On 2021/2/10 下午10:52, P J P wrote:
> | > From: Prasad J Pandit <pjp@fedoraproject.org>
> | >
> | > While processing transmit (tx) descriptors in process_tx_desc()
> | > various descriptor fields are not checked properly. This may lead
> | > to infinite loop like issue. Add checks to avoid them.
> | >
> | > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> | > Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
> | > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de>
> | > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> | > ---
> | >   hw/net/e1000.c | 6 ++++++
> | >   1 file changed, 6 insertions(+)
> |
> | I guess you post the wrong patch :) ?
>
> Wrong patch...?
>
> Thank you.


Yes, I think I sent you a patch a week ago. I think we should use that 
one instead of using assert() here?

Thanks


> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Alexander Bulekov Feb. 19, 2021, 3:05 a.m. UTC | #4
On 210210 2022, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While processing transmit (tx) descriptors in process_tx_desc()
> various descriptor fields are not checked properly. This may lead
> to infinite loop like issue. Add checks to avoid them.
> 

+CC Peter Maydell

Is this a DMA re-entracy/stack-overflow issue? IIRC the plan was to have
some sort of wider fix for these issues. There are bunch of these
reports floating around at this point, I believe.

> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
> Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/e1000.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index d8da2f6528..15949a3d64 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -667,9 +667,11 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
>  
>      addr = le64_to_cpu(dp->buffer_addr);
>      if (tp->cptse) {
> +        assert(tp->tso_props.hdr_len);
>          msh = tp->tso_props.hdr_len + tp->tso_props.mss;
>          do {
>              bytes = split_size;
> +            assert(msh > tp->size);
>              if (tp->size + bytes > msh)
>                  bytes = msh - tp->size;
>  
> @@ -681,22 +683,26 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
>                  memmove(tp->header, tp->data, tp->tso_props.hdr_len);
>              }
>              tp->size = sz;
> +            assert(tp->size);   /* sz may get truncated */
>              addr += bytes;
>              if (sz == msh) {
>                  xmit_seg(s);
>                  memmove(tp->data, tp->header, tp->tso_props.hdr_len);
>                  tp->size = tp->tso_props.hdr_len;
>              }
> +            assert(split_size >= bytes);
>              split_size -= bytes;
>          } while (bytes && split_size);
>      } else {
>          split_size = MIN(sizeof(tp->data) - tp->size, split_size);
> +        assert(tp->size && split_size);
>          pci_dma_read(d, addr, tp->data + tp->size, split_size);
>          tp->size += split_size;
>      }
>  
>      if (!(txd_lower & E1000_TXD_CMD_EOP))
>          return;
> +    assert(tp->size && tp->tso_props.hdr_len);
>      if (!(tp->cptse && tp->size < tp->tso_props.hdr_len)) {
>          xmit_seg(s);
>      }
> -- 
> 2.29.2
>
Prasad Pandit Feb. 19, 2021, 9:01 a.m. UTC | #5
+-- On Thu, 18 Feb 2021, Alexander Bulekov wrote --+
| Is this a DMA re-entracy/stack-overflow issue? IIRC the plan was to have 
| some sort of wider fix for these issues. There are bunch of these reports 
| floating around at this point, I believe.

* It is similar to the eepro100 one.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Peter Maydell Feb. 19, 2021, 10:27 a.m. UTC | #6
On Fri, 19 Feb 2021 at 03:06, Alexander Bulekov <alxndr@bu.edu> wrote:
>
> On 210210 2022, P J P wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > While processing transmit (tx) descriptors in process_tx_desc()
> > various descriptor fields are not checked properly. This may lead
> > to infinite loop like issue. Add checks to avoid them.
> >
>
> +CC Peter Maydell
>
> Is this a DMA re-entracy/stack-overflow issue? IIRC the plan was to have
> some sort of wider fix for these issues. There are bunch of these
> reports floating around at this point, I believe.

I have no idea, because the commit message for this patch does
not describe the failure in any detail at all and has no
links to any bug report or test case for the failures it
claims to be fixing...

-- PMM
diff mbox series

Patch

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d8da2f6528..15949a3d64 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -667,9 +667,11 @@  process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
 
     addr = le64_to_cpu(dp->buffer_addr);
     if (tp->cptse) {
+        assert(tp->tso_props.hdr_len);
         msh = tp->tso_props.hdr_len + tp->tso_props.mss;
         do {
             bytes = split_size;
+            assert(msh > tp->size);
             if (tp->size + bytes > msh)
                 bytes = msh - tp->size;
 
@@ -681,22 +683,26 @@  process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
                 memmove(tp->header, tp->data, tp->tso_props.hdr_len);
             }
             tp->size = sz;
+            assert(tp->size);   /* sz may get truncated */
             addr += bytes;
             if (sz == msh) {
                 xmit_seg(s);
                 memmove(tp->data, tp->header, tp->tso_props.hdr_len);
                 tp->size = tp->tso_props.hdr_len;
             }
+            assert(split_size >= bytes);
             split_size -= bytes;
         } while (bytes && split_size);
     } else {
         split_size = MIN(sizeof(tp->data) - tp->size, split_size);
+        assert(tp->size && split_size);
         pci_dma_read(d, addr, tp->data + tp->size, split_size);
         tp->size += split_size;
     }
 
     if (!(txd_lower & E1000_TXD_CMD_EOP))
         return;
+    assert(tp->size && tp->tso_props.hdr_len);
     if (!(tp->cptse && tp->size < tp->tso_props.hdr_len)) {
         xmit_seg(s);
     }