Re: [Qemu-devel] [PATCH] net: e1000e: fix an infinite loop issue

[Dmitry Fleytman]

> On 8 Feb 2017, at 12:17 PM, Li Qiang <address@hidden> wrote:
> 
> Hello Dmitry,
> 
> 
> 2017-02-08 18:01 GMT+08:00 Dmitry Fleytman <address@hidden>:
> 
>> On 8 Feb 2017, at 11:30 AM, Li Qiang <address@hidden> wrote:
>> 
>> Hello, 
>> 
>> 2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <address@hidden>:
>> 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 <address@hidden> wrote:
>> >
>> > From: Li Qiang <address@hidden>
>> >
>> > 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 <address@hidden>
>> > ---
>> > 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
>> >