| Message ID | 20220824110845.353435-1-f.ebner@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] hw/net/vmxnet3: allow VMXNET3_MAX_MTU itself as a value | expand |
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 > >
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
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 =
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(-)