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

[Mauro Matteo Cascella]

On Thu, Nov 5, 2020 at 11:56 AM Mauro Matteo Cascella
<mcascell@redhat.com> 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;
>      }
> --
> 2.26.2
>

Here is a simple PoC to reproduce the aforementioned issue via qtest:

cat << EOF | ./qemu-system-x86_64 -device e1000e -trace e1000e\*
-display none -qtest stdio -accel qtest
outl 0xcf8 0x80002010
outl 0xcfc 0xe0000000
outl 0xcf8 0x80002004
outw 0xcfc 0x7
write 0xe0003800 0x4 0x10001000
write 0xe0003808 0x4 0x00080000
write 0xe0000400 0x4 0xfa000400
write 0xe0002808 0x4 0x00080000
write 0xe0002818 0x4 0x7f000000       <== set RDT to 127
write 0xe0005008 0x4 0xfbffa3fa
write 0xe0000100 0x4 0x5a840000
write 0x100018 0x2 0x6705
write 0x10001b 0x1 0x01
write 0xe0003818 0x4 0x04000000
EOF

--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0