* [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value
@ 2022-08-24 11:08 Fiona Ebner
2022-08-25 2:37 ` Jason Wang
2022-08-25 7:12 ` P J P
0 siblings, 2 replies; 3+ messages in thread
From: Fiona Ebner @ 2022-08-24 11:08 UTC (permalink / raw)
To: qemu-devel; +Cc: dmitry.fleytman, jasowang, pjp
Fixes: d05dcd94ae ("net: vmxnet3: validate configuration values during activate (CVE-2021-20203)")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
I'm not familiar with this code, so really I'm asking: is the change
justified?
I tested the change and it seems to work, but I only have some rough
rationale for it, which is also why there's no commit message yet.
In the Linux kernel's net/core/dev.c, in dev_validate_mtu(), the upper
limit itself is a valid value:
if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) {
NL_SET_ERR_MSG(extack, "mtu greater than device maximum");
return -EINVAL;
}
and AFAICT in the case of the vmxnet3 driver, max_mtu is set to
VMXNET3_MAX_MTU (as defined in the kernel, which is 9000, same as in
QEMU).
Reported by one of our users running into the failing assert():
https://forum.proxmox.com/threads/114011/#post-492916
hw/net/vmxnet3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 0b7acf7f89..a2037583bf 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1441,7 +1441,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
vmxnet3_setup_rx_filtering(s);
/* Cache fields from shared memory */
s->mtu = VMXNET3_READ_DRV_SHARED32(d, s->drv_shmem, devRead.misc.mtu);
- assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);
+ assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu <= VMXNET3_MAX_MTU);
VMW_CFPRN("MTU is %u", s->mtu);
s->max_rx_frags =
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value
2022-08-24 11:08 [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value Fiona Ebner
@ 2022-08-25 2:37 ` Jason Wang
2022-08-25 7:12 ` P J P
1 sibling, 0 replies; 3+ messages in thread
From: Jason Wang @ 2022-08-25 2:37 UTC (permalink / raw)
To: Fiona Ebner; +Cc: qemu-devel, Dmitry Fleytman, pjp
On Wed, Aug 24, 2022 at 7:17 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Fixes: d05dcd94ae ("net: vmxnet3: validate configuration values during activate (CVE-2021-20203)")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> I'm not familiar with this code, so really I'm asking: is the change
> justified?
Patch looks good, but please re-submit with a formal one with
rationals via changelog.
Thanks
>
> I tested the change and it seems to work, but I only have some rough
> rationale for it, which is also why there's no commit message yet.
>
> In the Linux kernel's net/core/dev.c, in dev_validate_mtu(), the upper
> limit itself is a valid value:
> if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) {
> NL_SET_ERR_MSG(extack, "mtu greater than device maximum");
> return -EINVAL;
> }
> and AFAICT in the case of the vmxnet3 driver, max_mtu is set to
> VMXNET3_MAX_MTU (as defined in the kernel, which is 9000, same as in
> QEMU).
>
> Reported by one of our users running into the failing assert():
> https://forum.proxmox.com/threads/114011/#post-492916
>
> hw/net/vmxnet3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 0b7acf7f89..a2037583bf 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1441,7 +1441,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> vmxnet3_setup_rx_filtering(s);
> /* Cache fields from shared memory */
> s->mtu = VMXNET3_READ_DRV_SHARED32(d, s->drv_shmem, devRead.misc.mtu);
> - assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);
> + assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu <= VMXNET3_MAX_MTU);
> VMW_CFPRN("MTU is %u", s->mtu);
>
> s->max_rx_frags =
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value
2022-08-24 11:08 [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value Fiona Ebner
2022-08-25 2:37 ` Jason Wang
@ 2022-08-25 7:12 ` P J P
1 sibling, 0 replies; 3+ messages in thread
From: P J P @ 2022-08-25 7:12 UTC (permalink / raw)
To: qemu-devel, Fiona Ebner; +Cc: dmitry.fleytman, jasowang
On Wednesday, 24 August, 2022, 04:46:21 pm IST, Fiona Ebner <f.ebner@proxmox.com> wrote:
>Reported by one of our users running into the failing assert():
>https://forum.proxmox.com/threads/114011/#post-492916
>
>- assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu < VMXNET3_MAX_MTU);
>+ assert(VMXNET3_MIN_MTU <= s->mtu && s->mtu <= VMXNET3_MAX_MTU);
> VMW_CFPRN("MTU is %u", s->mtu);
>
* I wonder if setting s->mtu == buffer_upper_limit may lead to an out-of-bounds access issue?
* IIUC, VMXNET3_MAX_MTU OR s->mtu does not seem to be used to allocate and/or access packet buffer(s)
so above check might work, but still it does not seem clean, ie. it may lead to some confusion.
* Nonetheless, Jason has acked it, so that's good.
Thank you.
---
-P J P
http://feedmug.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-25 7:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-24 11:08 [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value Fiona Ebner
2022-08-25 2:37 ` Jason Wang
2022-08-25 7:12 ` P J P
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.