Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Jason Wang <jasowang@redhat.com>]

On 2020/11/30 下午10:12, Mauro Matteo Cascella wrote:
> On Mon, Nov 30, 2020 at 3:58 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/27 下午10:49, Mauro Matteo Cascella wrote:
>>> On Fri, Nov 27, 2020 at 6:21 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote:
>>>>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>>>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>>>>>> value accordingly.
>>>>>>>> Can this patch solve this issue in another way?
>>>>>>>>
>>>>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>>>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>>>>> So if RDT is odd, it looks to me the following codes in
>>>>>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>>>>>
>>>>>>
>>>>>>             base = e1000e_ring_head_descr(core, rxi);
>>>>>>
>>>>>>             pci_dma_read(d, base, &desc, core->rx_desc_len);
>>>>>>
>>>>>> Otherwise e1000e may try to read out of descriptor ring.
>>>>> Sorry, I'm not sure I understand what you mean. Isn't the base address
>>>>> computed from RDH? How can e1000e read out of the descriptor ring if
>>>>> RDT is odd?
>>>>>
>>>>>> Thanks
>>>>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>>>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>>>>>> value accordingly.
>>>>>>>> Can this patch solve this issue in another way?
>>>>>>>>
>>>>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>>>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>>>>> So if RDT is odd, it looks to me the following codes in
>>>>>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>>>>>
>>>>>>
>>>>>>             base = e1000e_ring_head_descr(core, rxi);
>>>>>>
>>>>>>             pci_dma_read(d, base, &desc, core->rx_desc_len);
>>>>>>
>>>>>> Otherwise e1000e may try to read out of descriptor ring.
>>>>>>
>>>>>> Thanks
>>>> Sorry, I meant RDH actually, when packet split descriptor is used, it
>>>> doesn't check whether DH exceeds DLEN?
>>>>
>>> When the packet split feature is used (i.e., count > 1) this patch
>>> basically sets RDH=RDT in case the increment would exceed RDT.
>>
>> Can software set RDH to an odd value? If not, I think we are probably fine.
>>
>> Thanks
>>
> Honestly I don't know the answer to your question, my guess is that it
> may be possible for a malicious/bogus guest to set RDH the same way as
> RDT.
>
> Thank you,
> --
> Mauro Matteo Cascella
> Red Hat Product Security
> PGP-Key ID: BB3410B0
>
>

Then I think we should fix that.

Thanks