Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()

[Jason Wang <jasowang@redhat.com>]

On 2020/11/10 下午5:06, Mauro Matteo Cascella wrote:
> On Mon, Nov 9, 2020 at 3:38 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/5 下午6:56, Mauro Matteo Cascella wrote:
>>> The e1000e_write_packet_to_guest() function iterates over a set of
>>> receive descriptors by advancing rx descriptor head register (RDH) from
>>> its initial value to rx descriptor tail register (RDT). The check in
>>> e1000e_ring_empty() is responsible for detecting whether RDH has reached
>>> RDT, terminating the loop if that's the case. Additional checks have
>>> been added in the past to deal with bogus values submitted by the guest
>>> to prevent possible infinite loop. This is done by "wrapping around" RDH
>>> at some point and detecting whether it assumes the original value during
>>> the loop.
>>>
>>> However, when e1000e is configured to use the packet split feature, RDH is
>>> incremented by two instead of one, as the packet split descriptors are
>>> 32 bytes while regular descriptors are 16 bytes. A malicious or buggy
>>> guest may set RDT to an odd value and transmit only null RX descriptors.
>>> This corner case would prevent RDH from ever matching RDT, leading to an
>>> infinite loop. This patch adds a check in e1000e_ring_advance() to make
>>> sure RDH does never exceed RDT.
>>>
>>> This issue was independently reported by Gaoning Pan (Zhejiang University)
>>> and Cheolwoo Myung.
>>>
>>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>> Reported-by: Gaoning Pan <gaoning.pgn@antgroup.com>
>>> Reported-by: Cheolwoo Myung <330cjfdn@gmail.com>
>>> ---
>>> References:
>>> https://git.qemu.org/?p=qemu.git;a=commit;h=dd793a74882477ca38d49e191110c17dfe
>>> https://git.qemu.org/?p=qemu.git;a=commit;h=4154c7e03fa55b4cf52509a83d50d6c09d743b7
>>> http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
>>>
>>>    hw/net/e1000e_core.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>>> index bcd186cac5..4c4d14b6ed 100644
>>> --- a/hw/net/e1000e_core.c
>>> +++ b/hw/net/e1000e_core.c
>>> @@ -831,6 +831,10 @@ e1000e_ring_advance(E1000ECore *core, const E1000E_RingInfo *r, uint32_t count)
>>>    {
>>>        core->mac[r->dh] += count;
>>>
>>> +    if (core->mac[r->dh] > core->mac[r->dt]) {
>>> +        core->mac[r->dh] = core->mac[r->dt];
>>> +    }
>>> +
>>>        if (core->mac[r->dh] * E1000_RING_DESC_LEN >= core->mac[r->dlen]) {
>>>            core->mac[r->dh] = 0;
>>>        }
> Hi Jason,
>
>> A question here.
>>
>> When count > 1, is this correct to reset dh here?
>>
>> Thanks
>>
> My understanding is that wrapping at (or above) RDLEN is the correct
> behavior regardless of count. I don't see why count > 1 should modify
> this behavior. I'm not sure, though. Anyway, this patch fixes the
> above reproducer, so I'm adding a Tested-by line here.
>
> Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
>
> Thank you,
> --
> Mauro Matteo Cascella
> Red Hat Product Security
> PGP-Key ID: BB3410B0
>

Right.

Applied.

Thanks