hw/net/e1000e_core.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
From: Li Qiang <liqiang6-s@360.cn>
This issue is the same as e1000 network card in this commit:
e1000: eliminate infinite loops on out-of-bounds transfer start.
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
hw/net/e1000e_core.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 2b11499..53f2b1d 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
struct e1000_tx_desc desc;
bool ide = false;
const E1000E_RingInfo *txi = txr->i;
- uint32_t cause = E1000_ICS_TXQE;
+ uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
+
if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
trace_e1000e_tx_disabled();
@@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
cause |= e1000e_txdesc_writeback(core, base, &desc, &ide, txi->idx);
e1000e_ring_advance(core, txi, 1);
+
+ /*
+ * The following avoid infinite loop, just as the e1000
+ */
+ if (core->mac[txi->dh] == tdh_start ||
+ tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
+ break;
+ }
}
if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
@@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
size_t desc_size;
size_t desc_offset = 0;
size_t iov_ofs = 0;
+ uint32_t rdh_start;
struct iovec *iov = net_rx_pkt_get_iovec(pkt);
size_t size = net_rx_pkt_get_total_len(pkt);
@@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
rxi = rxr->i;
+ rdh_start = core->mac[rxi->dh];
do {
hwaddr ba[MAX_PS_BUFFERS];
@@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
e1000e_ring_advance(core, rxi,
core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
+ if (core->mac[rxi->dh] == rdh_start ||
+ rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
+ break;
+ }
} while (desc_offset < total_size);
e1000e_update_rx_stats(core, size, total_size);
--
1.8.3.1
Hello,
Thanks for the patch!
The problem of infinite loop indeed exists in e1000e, however it is different from the one fixed in e1000.
Please find my comments inline.
~Dmitry
> On 7 Feb 2017, at 11:43 AM, Li Qiang <liq3ea@gmail.com> wrote:
>
> From: Li Qiang <liqiang6-s@360.cn>
>
> This issue is the same as e1000 network card in this commit:
> e1000: eliminate infinite loops on out-of-bounds transfer start.
>
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
> hw/net/e1000e_core.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 2b11499..53f2b1d 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
> struct e1000_tx_desc desc;
> bool ide = false;
> const E1000E_RingInfo *txi = txr->i;
> - uint32_t cause = E1000_ICS_TXQE;
> + uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
> +
>
> if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> trace_e1000e_tx_disabled();
> @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
> cause |= e1000e_txdesc_writeback(core, base, &desc, &ide, txi->idx);
>
> e1000e_ring_advance(core, txi, 1);
> +
> + /*
> + * The following avoid infinite loop, just as the e1000
> + */
> + if (core->mac[txi->dh] == tdh_start ||
> + tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
> + break;
> + }
Part of this validity check, namely
tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN)
already done by e1000e_ring_advance(), therefore not needed here.
The second part - full wraparound protection is relevant, but it fixes more symptoms than the cause.
The only possible cause for full wraparound is r->dt (tail) value bigger or equal to r->dlen (length),
so I would suggest to check for this in e1000e_ring_empty() and cover both TX and RX cases at once.
> }
>
> if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
> @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
> size_t desc_size;
> size_t desc_offset = 0;
> size_t iov_ofs = 0;
> + uint32_t rdh_start;
>
> struct iovec *iov = net_rx_pkt_get_iovec(pkt);
> size_t size = net_rx_pkt_get_total_len(pkt);
> @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
> bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
>
> rxi = rxr->i;
> + rdh_start = core->mac[rxi->dh];
>
> do {
> hwaddr ba[MAX_PS_BUFFERS];
> @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
> e1000e_ring_advance(core, rxi,
> core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
>
> + if (core->mac[rxi->dh] == rdh_start ||
> + rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
> + break;
> + }
> } while (desc_offset < total_size);
>
> e1000e_update_rx_stats(core, size, total_size);
> --
> 1.8.3.1
>
Hello,
2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
> Hello,
>
> Thanks for the patch!
>
> The problem of infinite loop indeed exists in e1000e, however it is
> different from the one fixed in e1000.
> Please find my comments inline.
>
>
If I read the code correctly, I think this issue is the same. Could you
please explain more? Thanks.
> ~Dmitry
>
> > On 7 Feb 2017, at 11:43 AM, Li Qiang <liq3ea@gmail.com> wrote:
> >
> > From: Li Qiang <liqiang6-s@360.cn>
> >
> > This issue is the same as e1000 network card in this commit:
> > e1000: eliminate infinite loops on out-of-bounds transfer start.
> >
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> > hw/net/e1000e_core.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index 2b11499..53f2b1d 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const
> E1000E_TxRing *txr)
> > struct e1000_tx_desc desc;
> > bool ide = false;
> > const E1000E_RingInfo *txi = txr->i;
> > - uint32_t cause = E1000_ICS_TXQE;
> > + uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
> > +
> >
> > if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> > trace_e1000e_tx_disabled();
> > @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const
> E1000E_TxRing *txr)
> > cause |= e1000e_txdesc_writeback(core, base, &desc, &ide,
> txi->idx);
> >
> > e1000e_ring_advance(core, txi, 1);
> > +
> > + /*
> > + * The following avoid infinite loop, just as the e1000
> > + */
> > + if (core->mac[txi->dh] == tdh_start ||
> > + tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
> > + break;
> > + }
>
> Part of this validity check, namely
> tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN)
> already done by e1000e_ring_advance(), therefore not needed here.
>
>
e1000e_ring_advance() check the added r->dh. Not the original one.
> The second part - full wraparound protection is relevant, but it fixes
> more symptoms than the cause.
> The only possible cause for full wraparound is r->dt (tail) value bigger
> or equal to r->dlen (length),
> so I would suggest to check for this in e1000e_ring_empty() and cover both
> TX and RX cases at once.
>
>
Do you mean 'core->mac[r->dt] >= core->mac[r->dlen]' indicate an empty ring?
> > }
> >
> > if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
> > @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core,
> struct NetRxPkt *pkt,
> > size_t desc_size;
> > size_t desc_offset = 0;
> > size_t iov_ofs = 0;
> > + uint32_t rdh_start;
> >
> > struct iovec *iov = net_rx_pkt_get_iovec(pkt);
> > size_t size = net_rx_pkt_get_total_len(pkt);
> > @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core,
> struct NetRxPkt *pkt,
> > bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
> >
> > rxi = rxr->i;
> > + rdh_start = core->mac[rxi->dh];
> >
> > do {
> > hwaddr ba[MAX_PS_BUFFERS];
> > @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core,
> struct NetRxPkt *pkt,
> > e1000e_ring_advance(core, rxi,
> > core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
> >
> > + if (core->mac[rxi->dh] == rdh_start ||
> > + rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
> > + break;
> > + }
> > } while (desc_offset < total_size);
> >
> > e1000e_update_rx_stats(core, size, total_size);
> > --
> > 1.8.3.1
> >
>
>
> On 8 Feb 2017, at 11:30 AM, Li Qiang <liq3ea@gmail.com> wrote:
>
> Hello,
>
> 2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>:
> Hello,
>
> Thanks for the patch!
>
> The problem of infinite loop indeed exists in e1000e, however it is different from the one fixed in e1000.
> Please find my comments inline.
>
>
> If I read the code correctly, I think this issue is the same. Could you please explain more? Thanks.
I meant that code changes needed to fix it are slightly different.
>
> ~Dmitry
>
> > On 7 Feb 2017, at 11:43 AM, Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>> wrote:
> >
> > From: Li Qiang <liqiang6-s@360.cn <mailto:liqiang6-s@360.cn>>
> >
> > This issue is the same as e1000 network card in this commit:
> > e1000: eliminate infinite loops on out-of-bounds transfer start.
> >
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn <mailto:liqiang6-s@360.cn>>
> > ---
> > hw/net/e1000e_core.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index 2b11499..53f2b1d 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
> > struct e1000_tx_desc desc;
> > bool ide = false;
> > const E1000E_RingInfo *txi = txr->i;
> > - uint32_t cause = E1000_ICS_TXQE;
> > + uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
> > +
> >
> > if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> > trace_e1000e_tx_disabled();
> > @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
> > cause |= e1000e_txdesc_writeback(core, base, &desc, &ide, txi->idx);
> >
> > e1000e_ring_advance(core, txi, 1);
> > +
> > + /*
> > + * The following avoid infinite loop, just as the e1000
> > + */
> > + if (core->mac[txi->dh] == tdh_start ||
> > + tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
> > + break;
> > + }
>
> Part of this validity check, namely
> tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN)
> already done by e1000e_ring_advance(), therefore not needed here.
>
>
> e1000e_ring_advance() check the added r->dh. Not the original one.
Oh, I see now. Right, it does not make sense to check for wraparound in case original TDH was out of bounds.
Now I see there is one more problem - in case TDH is out of the ring, e1000e_ring_head_descr()
called from e1000e_start_xmit() will do out of bounds read, so TDH should be validated prior to
reading the descriptor as well.
Similar problem exists on RX also.
Besides that, as I see now, e1000e_ring_empty() is not called on RX
so validation should be done separately for TX and RX rings.
A function like e1000e_ring_validate() is needed...
>
> The second part - full wraparound protection is relevant, but it fixes more symptoms than the cause.
> The only possible cause for full wraparound is r->dt (tail) value bigger or equal to r->dlen (length),
> so I would suggest to check for this in e1000e_ring_empty() and cover both TX and RX cases at once.
>
>
> Do you mean 'core->mac[r->dt] >= core->mac[r->dlen]' indicate an empty ring?
>
> > }
> >
> > if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
> > @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
> > size_t desc_size;
> > size_t desc_offset = 0;
> > size_t iov_ofs = 0;
> > + uint32_t rdh_start;
> >
> > struct iovec *iov = net_rx_pkt_get_iovec(pkt);
> > size_t size = net_rx_pkt_get_total_len(pkt);
> > @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
> > bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
> >
> > rxi = rxr->i;
> > + rdh_start = core->mac[rxi->dh];
> >
> > do {
> > hwaddr ba[MAX_PS_BUFFERS];
> > @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
> > e1000e_ring_advance(core, rxi,
> > core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
> >
> > + if (core->mac[rxi->dh] == rdh_start ||
> > + rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
> > + break;
> > + }
> > } while (desc_offset < total_size);
> >
> > e1000e_update_rx_stats(core, size, total_size);
> > --
> > 1.8.3.1
> >
Hello Dmitry,
2017-02-08 18:01 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
>
> On 8 Feb 2017, at 11:30 AM, Li Qiang <liq3ea@gmail.com> wrote:
>
> Hello,
>
> 2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
>
>> Hello,
>>
>> Thanks for the patch!
>>
>> The problem of infinite loop indeed exists in e1000e, however it is
>> different from the one fixed in e1000.
>> Please find my comments inline.
>>
>>
> If I read the code correctly, I think this issue is the same. Could you
> please explain more? Thanks.
>
>
> I meant that code changes needed to fix it are slightly different.
>
Got.
>
>
>
>> ~Dmitry
>>
>> > On 7 Feb 2017, at 11:43 AM, Li Qiang <liq3ea@gmail.com> wrote:
>> >
>> > From: Li Qiang <liqiang6-s@360.cn>
>> >
>> > This issue is the same as e1000 network card in this commit:
>> > e1000: eliminate infinite loops on out-of-bounds transfer start.
>> >
>> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
>> > ---
>> > hw/net/e1000e_core.c | 17 ++++++++++++++++-
>> > 1 file changed, 16 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> > index 2b11499..53f2b1d 100644
>> > --- a/hw/net/e1000e_core.c
>> > +++ b/hw/net/e1000e_core.c
>> > @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const
>> E1000E_TxRing *txr)
>> > struct e1000_tx_desc desc;
>> > bool ide = false;
>> > const E1000E_RingInfo *txi = txr->i;
>> > - uint32_t cause = E1000_ICS_TXQE;
>> > + uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
>> > +
>> >
>> > if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
>> > trace_e1000e_tx_disabled();
>> > @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const
>> E1000E_TxRing *txr)
>> > cause |= e1000e_txdesc_writeback(core, base, &desc, &ide,
>> txi->idx);
>> >
>> > e1000e_ring_advance(core, txi, 1);
>> > +
>> > + /*
>> > + * The following avoid infinite loop, just as the e1000
>> > + */
>> > + if (core->mac[txi->dh] == tdh_start ||
>> > + tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
>> > + break;
>> > + }
>>
>> Part of this validity check, namely
>> tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN)
>> already done by e1000e_ring_advance(), therefore not needed here.
>>
>>
> e1000e_ring_advance() check the added r->dh. Not the original one.
>
>
> Oh, I see now. Right, it does not make sense to check for wraparound in
> case original TDH was out of bounds.
>
> Now I see there is one more problem - in case TDH is out of the
> ring, e1000e_ring_head_descr()
> called from e1000e_start_xmit() will do out of bounds read, so TDH should
> be validated prior to
> reading the descriptor as well.
>
>
IIUC you mean pci_dma_read will do oob read, right? I think this is not a
issue as this is reading data from guest
And guest can provide arbitrary data. For example, just provide 10 bytes
buffer, but tell the qemu it has 1000.
> Similar problem exists on RX also.
>
> Besides that, as I see now, e1000e_ring_empty() is not called on RX
> so validation should be done separately for TX and RX rings.
>
> A function like e1000e_ring_validate() is needed...
>
>
>
>> The second part - full wraparound protection is relevant, but it fixes
>> more symptoms than the cause.
>> The only possible cause for full wraparound is r->dt (tail) value bigger
>> or equal to r->dlen (length),
>> so I would suggest to check for this in e1000e_ring_empty() and cover
>> both TX and RX cases at once.
>>
>>
> Do you mean 'core->mac[r->dt] >= core->mac[r->dlen]' indicate an empty
> ring?
>
>
>
>
So I still confused by this 'full wrapparound', any more explain?
> > }
>> >
>> > if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
>> > @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core,
>> struct NetRxPkt *pkt,
>> > size_t desc_size;
>> > size_t desc_offset = 0;
>> > size_t iov_ofs = 0;
>> > + uint32_t rdh_start;
>> >
>> > struct iovec *iov = net_rx_pkt_get_iovec(pkt);
>> > size_t size = net_rx_pkt_get_total_len(pkt);
>> > @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core,
>> struct NetRxPkt *pkt,
>> > bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
>> >
>> > rxi = rxr->i;
>> > + rdh_start = core->mac[rxi->dh];
>> >
>> > do {
>> > hwaddr ba[MAX_PS_BUFFERS];
>> > @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core,
>> struct NetRxPkt *pkt,
>> > e1000e_ring_advance(core, rxi,
>> > core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
>> >
>> > + if (core->mac[rxi->dh] == rdh_start ||
>> > + rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
>> > + break;
>> > + }
>> > } while (desc_offset < total_size);
>> >
>> > e1000e_update_rx_stats(core, size, total_size);
>> > --
>> > 1.8.3.1
>> >
>>
>
>
> On 8 Feb 2017, at 12:17 PM, Li Qiang <liq3ea@gmail.com> wrote:
>
> Hello Dmitry,
>
>
> 2017-02-08 18:01 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
>
>> On 8 Feb 2017, at 11:30 AM, Li Qiang <liq3ea@gmail.com> wrote:
>>
>> Hello,
>>
>> 2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
>> Hello,
>>
>> Thanks for the patch!
>>
>> The problem of infinite loop indeed exists in e1000e, however it is different from the one fixed in e1000.
>> Please find my comments inline.
>>
>>
>> If I read the code correctly, I think this issue is the same. Could you please explain more? Thanks.
>
> I meant that code changes needed to fix it are slightly different.
>
> Got.
>
>
>>
>> ~Dmitry
>>
>> > On 7 Feb 2017, at 11:43 AM, Li Qiang <liq3ea@gmail.com> wrote:
>> >
>> > From: Li Qiang <liqiang6-s@360.cn>
>> >
>> > This issue is the same as e1000 network card in this commit:
>> > e1000: eliminate infinite loops on out-of-bounds transfer start.
>> >
>> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
>> > ---
>> > hw/net/e1000e_core.c | 17 ++++++++++++++++-
>> > 1 file changed, 16 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> > index 2b11499..53f2b1d 100644
>> > --- a/hw/net/e1000e_core.c
>> > +++ b/hw/net/e1000e_core.c
>> > @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
>> > struct e1000_tx_desc desc;
>> > bool ide = false;
>> > const E1000E_RingInfo *txi = txr->i;
>> > - uint32_t cause = E1000_ICS_TXQE;
>> > + uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE;
>> > +
>> >
>> > if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
>> > trace_e1000e_tx_disabled();
>> > @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
>> > cause |= e1000e_txdesc_writeback(core, base, &desc, &ide, txi->idx);
>> >
>> > e1000e_ring_advance(core, txi, 1);
>> > +
>> > + /*
>> > + * The following avoid infinite loop, just as the e1000
>> > + */
>> > + if (core->mac[txi->dh] == tdh_start ||
>> > + tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) {
>> > + break;
>> > + }
>>
>> Part of this validity check, namely
>> tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN)
>> already done by e1000e_ring_advance(), therefore not needed here.
>>
>>
>> e1000e_ring_advance() check the added r->dh. Not the original one.
>
> Oh, I see now. Right, it does not make sense to check for wraparound in case original TDH was out of bounds.
>
> Now I see there is one more problem - in case TDH is out of the ring, e1000e_ring_head_descr()
> called from e1000e_start_xmit() will do out of bounds read, so TDH should be validated prior to
> reading the descriptor as well.
>
>
> IIUC you mean pci_dma_read will do oob read, right? I think this is not a issue as this is reading data from guest
> And guest can provide arbitrary data. For example, just provide 10 bytes buffer, but tell the qemu it has 1000.
Agree.
>
> Similar problem exists on RX also.
>
> Besides that, as I see now, e1000e_ring_empty() is not called on RX
> so validation should be done separately for TX and RX rings.
>
> A function like e1000e_ring_validate() is needed...
>
>>
>> The second part - full wraparound protection is relevant, but it fixes more symptoms than the cause.
>> The only possible cause for full wraparound is r->dt (tail) value bigger or equal to r->dlen (length),
>> so I would suggest to check for this in e1000e_ring_empty() and cover both TX and RX cases at once.
>>
>>
>> Do you mean 'core->mac[r->dt] >= core->mac[r->dlen]' indicate an empty ring?
>>
>
> So I still confused by this 'full wrapparound', any more explain?
Yes, I would indicate an empty ring when r->dt in invalid. That would do the job.
Having e1000e_ring_empty() fixed, you might call it from
e1000e_write_packet_to_guest() to fix RX issues as well.
>
>> > }
>> >
>> > if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
>> > @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
>> > size_t desc_size;
>> > size_t desc_offset = 0;
>> > size_t iov_ofs = 0;
>> > + uint32_t rdh_start;
>> >
>> > struct iovec *iov = net_rx_pkt_get_iovec(pkt);
>> > size_t size = net_rx_pkt_get_total_len(pkt);
>> > @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
>> > bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len);
>> >
>> > rxi = rxr->i;
>> > + rdh_start = core->mac[rxi->dh];
>> >
>> > do {
>> > hwaddr ba[MAX_PS_BUFFERS];
>> > @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
>> > e1000e_ring_advance(core, rxi,
>> > core->rx_desc_len / E1000_MIN_RX_DESC_LEN);
>> >
>> > + if (core->mac[rxi->dh] == rdh_start ||
>> > + rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) {
>> > + break;
>> > + }
>> > } while (desc_offset < total_size);
>> >
>> > e1000e_update_rx_stats(core, size, total_size);
>> > --
>> > 1.8.3.1
>> >
© 2016 - 2024 Red Hat, Inc.