* [PATCH] net: e1000: check transmit descriptor field values
@ 2021-02-10 14:52 P J P
2021-02-18 5:53 ` Jason Wang
2021-02-19 3:05 ` Alexander Bulekov
0 siblings, 2 replies; 7+ messages in thread
From: P J P @ 2021-02-10 14:52 UTC (permalink / raw)
To: Jason Wang
Cc: Alexander Bulekov, Cheolwoo Myung, Ruhr-University Bochum,
QEMU Developers, Prasad J Pandit
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(+)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] net: e1000: check transmit descriptor field values
2021-02-10 14:52 [PATCH] net: e1000: check transmit descriptor field values P J P
@ 2021-02-18 5:53 ` Jason Wang
2021-02-18 7:47 ` P J P
2021-02-19 3:05 ` Alexander Bulekov
1 sibling, 1 reply; 7+ messages in thread
From: Jason Wang @ 2021-02-18 5:53 UTC (permalink / raw)
To: P J P
Cc: Alexander Bulekov, Cheolwoo Myung, Ruhr-University Bochum,
QEMU Developers, Prasad J Pandit
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);
> }
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] net: e1000: check transmit descriptor field values
2021-02-18 5:53 ` Jason Wang
@ 2021-02-18 7:47 ` P J P
2021-02-19 2:53 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: P J P @ 2021-02-18 7:47 UTC (permalink / raw)
To: Jason Wang
Cc: Alexander Bulekov, Cheolwoo Myung, Ruhr-University Bochum,
QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 830 bytes --]
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: e1000: check transmit descriptor field values
2021-02-18 7:47 ` P J P
@ 2021-02-19 2:53 ` Jason Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2021-02-19 2:53 UTC (permalink / raw)
To: P J P
Cc: Alexander Bulekov, Cheolwoo Myung, Ruhr-University Bochum,
QEMU Developers
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: e1000: check transmit descriptor field values
2021-02-10 14:52 [PATCH] net: e1000: check transmit descriptor field values P J P
2021-02-18 5:53 ` Jason Wang
@ 2021-02-19 3:05 ` Alexander Bulekov
2021-02-19 9:01 ` P J P
2021-02-19 10:27 ` Peter Maydell
1 sibling, 2 replies; 7+ messages in thread
From: Alexander Bulekov @ 2021-02-19 3:05 UTC (permalink / raw)
To: P J P
Cc: Peter Maydell, Prasad J Pandit, Cheolwoo Myung, QEMU Developers,
Ruhr-University Bochum, Jason Wang
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] net: e1000: check transmit descriptor field values
2021-02-19 3:05 ` Alexander Bulekov
@ 2021-02-19 9:01 ` P J P
2021-02-19 10:27 ` Peter Maydell
1 sibling, 0 replies; 7+ messages in thread
From: P J P @ 2021-02-19 9:01 UTC (permalink / raw)
To: Alexander Bulekov
Cc: Peter Maydell, Jason Wang, Ruhr-University Bochum,
QEMU Developers, Cheolwoo Myung
+-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: e1000: check transmit descriptor field values
2021-02-19 3:05 ` Alexander Bulekov
2021-02-19 9:01 ` P J P
@ 2021-02-19 10:27 ` Peter Maydell
1 sibling, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2021-02-19 10:27 UTC (permalink / raw)
To: Alexander Bulekov
Cc: Prasad J Pandit, Cheolwoo Myung, QEMU Developers, P J P,
Ruhr-University Bochum, Jason Wang
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-19 10:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 14:52 [PATCH] net: e1000: check transmit descriptor field values P J P
2021-02-18 5:53 ` Jason Wang
2021-02-18 7:47 ` P J P
2021-02-19 2:53 ` Jason Wang
2021-02-19 3:05 ` Alexander Bulekov
2021-02-19 9:01 ` P J P
2021-02-19 10:27 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.