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
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);
> }
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
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
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
>
+-- 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
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
© 2016 - 2024 Red Hat, Inc.