| Message ID | 20230725113632.2386874-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-8.1] hw/rdma/vmw/pvrdma_cmd: Use correct struct in query_port() | expand |
On 25/7/23 13:36, Peter Maydell wrote: > In query_port() we pass the address of a local pvrdma_port_attr > struct to the rdma_query_backend_port() function. Unfortunately, > rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, > and the two are not the same length. > > Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes > long, and ibv_port_attr is 52 bytes, because it has a few extra > fields at the end. > > Fortunately, all we do with the attrs struct after the call is to > read a few specific fields out of it which are all at the same > offsets in both structs, so we can simply make the local variable the > correct type. This also lets us drop the cast (which should have > been a bit of a warning flag that we were doing something wrong > here). Fortunate but also kind of amusing :) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I don't know anything about the rdma code so this fix is based > purely on looking at the code, and is untested beyond just > make check/make check-avocado. > --- > hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-)
On Tue, 25 Jul 2023 at 12:36, Peter Maydell <peter.maydell@linaro.org> wrote: > > In query_port() we pass the address of a local pvrdma_port_attr > struct to the rdma_query_backend_port() function. Unfortunately, > rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, > and the two are not the same length. > > Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes > long, and ibv_port_attr is 52 bytes, because it has a few extra > fields at the end. > > Fortunately, all we do with the attrs struct after the call is to > read a few specific fields out of it which are all at the same > offsets in both structs, so we can simply make the local variable the > correct type. This also lets us drop the cast (which should have > been a bit of a warning flag that we were doing something wrong > here). > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I don't know anything about the rdma code so this fix is based > purely on looking at the code, and is untested beyond just > make check/make check-avocado. > --- > hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > index c6ed0259821..d31c1875938 100644 > --- a/hw/rdma/vmw/pvrdma_cmd.c > +++ b/hw/rdma/vmw/pvrdma_cmd.c > @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req, > { > struct pvrdma_cmd_query_port *cmd = &req->query_port; > struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp; > - struct pvrdma_port_attr attrs = {}; > + struct ibv_port_attr attrs = {}; > > if (cmd->port_num > MAX_PORTS) { > return -EINVAL; > } > > - if (rdma_backend_query_port(&dev->backend_dev, > - (struct ibv_port_attr *)&attrs)) { > + if (rdma_backend_query_port(&dev->backend_dev, &attrs)) { > return -ENOMEM; > } Ping for review/testing by the rdma folks, please ? Whose tree should this patch go through? thanks -- PMM
Ping^2 for review/pickup by the rdma folks, please? thanks -- PMM On Tue, 29 Aug 2023 at 16:49, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 25 Jul 2023 at 12:36, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > In query_port() we pass the address of a local pvrdma_port_attr > > struct to the rdma_query_backend_port() function. Unfortunately, > > rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, > > and the two are not the same length. > > > > Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes > > long, and ibv_port_attr is 52 bytes, because it has a few extra > > fields at the end. > > > > Fortunately, all we do with the attrs struct after the call is to > > read a few specific fields out of it which are all at the same > > offsets in both structs, so we can simply make the local variable the > > correct type. This also lets us drop the cast (which should have > > been a bit of a warning flag that we were doing something wrong > > here). > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > I don't know anything about the rdma code so this fix is based > > purely on looking at the code, and is untested beyond just > > make check/make check-avocado. > > --- > > hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > > index c6ed0259821..d31c1875938 100644 > > --- a/hw/rdma/vmw/pvrdma_cmd.c > > +++ b/hw/rdma/vmw/pvrdma_cmd.c > > @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req, > > { > > struct pvrdma_cmd_query_port *cmd = &req->query_port; > > struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp; > > - struct pvrdma_port_attr attrs = {}; > > + struct ibv_port_attr attrs = {}; > > > > if (cmd->port_num > MAX_PORTS) { > > return -EINVAL; > > } > > > > - if (rdma_backend_query_port(&dev->backend_dev, > > - (struct ibv_port_attr *)&attrs)) { > > + if (rdma_backend_query_port(&dev->backend_dev, &attrs)) { > > return -ENOMEM; > > } > > Ping for review/testing by the rdma folks, please ? > Whose tree should this patch go through?
Reviewed-by: Thomas Huth <thuth@redhat.com> Maybe this could go via qemu-trivial? On 12/09/2023 16.08, Peter Maydell wrote: > Ping^2 for review/pickup by the rdma folks, please? Is anybody still using this subsystem? ... if not, then it's maybe time to set this on the deprecation list? ... just my 0.02 €. Thomas > > On Tue, 29 Aug 2023 at 16:49, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Tue, 25 Jul 2023 at 12:36, Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>> In query_port() we pass the address of a local pvrdma_port_attr >>> struct to the rdma_query_backend_port() function. Unfortunately, >>> rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, >>> and the two are not the same length. >>> >>> Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes >>> long, and ibv_port_attr is 52 bytes, because it has a few extra >>> fields at the end. >>> >>> Fortunately, all we do with the attrs struct after the call is to >>> read a few specific fields out of it which are all at the same >>> offsets in both structs, so we can simply make the local variable the >>> correct type. This also lets us drop the cast (which should have >>> been a bit of a warning flag that we were doing something wrong >>> here). >>> >>> Cc: qemu-stable@nongnu.org >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> I don't know anything about the rdma code so this fix is based >>> purely on looking at the code, and is untested beyond just >>> make check/make check-avocado. >>> --- >>> hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c >>> index c6ed0259821..d31c1875938 100644 >>> --- a/hw/rdma/vmw/pvrdma_cmd.c >>> +++ b/hw/rdma/vmw/pvrdma_cmd.c >>> @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req, >>> { >>> struct pvrdma_cmd_query_port *cmd = &req->query_port; >>> struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp; >>> - struct pvrdma_port_attr attrs = {}; >>> + struct ibv_port_attr attrs = {}; >>> >>> if (cmd->port_num > MAX_PORTS) { >>> return -EINVAL; >>> } >>> >>> - if (rdma_backend_query_port(&dev->backend_dev, >>> - (struct ibv_port_attr *)&attrs)) { >>> + if (rdma_backend_query_port(&dev->backend_dev, &attrs)) { >>> return -ENOMEM; >>> } >> >> Ping for review/testing by the rdma folks, please ? >> Whose tree should this patch go through? >
22.09.2023 18:05, Thomas Huth wrote: > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > Maybe this could go via qemu-trivial? > > On 12/09/2023 16.08, Peter Maydell wrote: >> Ping^2 for review/pickup by the rdma folks, please? > > Is anybody still using this subsystem? ... if not, then it's maybe time to set this on the deprecation list? ... just my 0.02 €. I applied this to my trivial-patches tree for now. There were several security issues in this area, I think one of them is still open with a patch posted to the list but no one were able to review it because the code is rather scary (iirc it was Phil who tried to review it but failed). Here's what I have in debian for quite some time: # pvrdma is an extension/optimisation for vmxnet3 vmware virtual network # adapter. This piece of code seems to be buggy and poorly maintained, # resulting in numerous security issues which comes unfixed for long time. # This device isn't native for qemu. # Just disable it for now. common_configure_opts += --disable-pvrdma So yes, it smells like deprecating it is a way to go. FWIW. /mjt >> On Tue, 29 Aug 2023 at 16:49, Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>> On Tue, 25 Jul 2023 at 12:36, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> >>>> In query_port() we pass the address of a local pvrdma_port_attr >>>> struct to the rdma_query_backend_port() function. Unfortunately, >>>> rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, >>>> and the two are not the same length. >>>> >>>> Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes >>>> long, and ibv_port_attr is 52 bytes, because it has a few extra >>>> fields at the end. >>>> >>>> Fortunately, all we do with the attrs struct after the call is to >>>> read a few specific fields out of it which are all at the same >>>> offsets in both structs, so we can simply make the local variable the >>>> correct type. This also lets us drop the cast (which should have >>>> been a bit of a warning flag that we were doing something wrong >>>> here). >>>> >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>>> --- >>>> I don't know anything about the rdma code so this fix is based >>>> purely on looking at the code, and is untested beyond just >>>> make check/make check-avocado. >>>> --- >>>> hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- >>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c >>>> index c6ed0259821..d31c1875938 100644 >>>> --- a/hw/rdma/vmw/pvrdma_cmd.c >>>> +++ b/hw/rdma/vmw/pvrdma_cmd.c >>>> @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req, >>>> { >>>> struct pvrdma_cmd_query_port *cmd = &req->query_port; >>>> struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp; >>>> - struct pvrdma_port_attr attrs = {}; >>>> + struct ibv_port_attr attrs = {}; >>>> >>>> if (cmd->port_num > MAX_PORTS) { >>>> return -EINVAL; >>>> } >>>> >>>> - if (rdma_backend_query_port(&dev->backend_dev, >>>> - (struct ibv_port_attr *)&attrs)) { >>>> + if (rdma_backend_query_port(&dev->backend_dev, &attrs)) { >>>> return -ENOMEM; >>>> } >>> >>> Ping for review/testing by the rdma folks, please ? >>> Whose tree should this patch go through? >> > >
On 25/07/2023 13.36, Peter Maydell wrote: > In query_port() we pass the address of a local pvrdma_port_attr > struct to the rdma_query_backend_port() function. Unfortunately, > rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, > and the two are not the same length. > > Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes > long, and ibv_port_attr is 52 bytes, because it has a few extra > fields at the end. > > Fortunately, all we do with the attrs struct after the call is to > read a few specific fields out of it which are all at the same > offsets in both structs, so we can simply make the local variable the > correct type. This also lets us drop the cast (which should have > been a bit of a warning flag that we were doing something wrong > here). > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I don't know anything about the rdma code so this fix is based > purely on looking at the code, and is untested beyond just > make check/make check-avocado. > --- > hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c > index c6ed0259821..d31c1875938 100644 > --- a/hw/rdma/vmw/pvrdma_cmd.c > +++ b/hw/rdma/vmw/pvrdma_cmd.c > @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req, > { > struct pvrdma_cmd_query_port *cmd = &req->query_port; > struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp; > - struct pvrdma_port_attr attrs = {}; > + struct ibv_port_attr attrs = {}; > > if (cmd->port_num > MAX_PORTS) { > return -EINVAL; > } > > - if (rdma_backend_query_port(&dev->backend_dev, > - (struct ibv_port_attr *)&attrs)) { > + if (rdma_backend_query_port(&dev->backend_dev, &attrs)) { > return -ENOMEM; > } > Hi Peter, this seems to fail with Clang: ../../devel/qemu/hw/rdma/vmw/pvrdma_cmd.c:144:59: error: implicit conversion from enumeration type 'enum ibv_port_state' to different enumeration type 'enum pvrdma_port_state' [-Werror,-Wenum-conversion] resp->attrs.state = dev->func0->device_active ? attrs.state : ~ ~~~~~~^~~~~ ../../devel/qemu/hw/rdma/vmw/pvrdma_cmd.c:146:33: error: implicit conversion from enumeration type 'enum ibv_mtu' to different enumeration type 'enum pvrdma_mtu' [-Werror,-Wenum-conversion] resp->attrs.max_mtu = attrs.max_mtu; ~ ~~~~~~^~~~~~~ ../../devel/qemu/hw/rdma/vmw/pvrdma_cmd.c:147:36: error: implicit conversion from enumeration type 'enum ibv_mtu' to different enumeration type 'enum pvrdma_mtu' [-Werror,-Wenum-conversion] resp->attrs.active_mtu = attrs.active_mtu; ~ ~~~~~~^~~~~~~~~~ 3 errors generated. Could you please have a look ? Thomas
diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c index c6ed0259821..d31c1875938 100644 --- a/hw/rdma/vmw/pvrdma_cmd.c +++ b/hw/rdma/vmw/pvrdma_cmd.c @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req, { struct pvrdma_cmd_query_port *cmd = &req->query_port; struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp; - struct pvrdma_port_attr attrs = {}; + struct ibv_port_attr attrs = {}; if (cmd->port_num > MAX_PORTS) { return -EINVAL; } - if (rdma_backend_query_port(&dev->backend_dev, - (struct ibv_port_attr *)&attrs)) { + if (rdma_backend_query_port(&dev->backend_dev, &attrs)) { return -ENOMEM; }
In query_port() we pass the address of a local pvrdma_port_attr struct to the rdma_query_backend_port() function. Unfortunately, rdma_backend_query_port() wants a pointer to a struct ibv_port_attr, and the two are not the same length. Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes long, and ibv_port_attr is 52 bytes, because it has a few extra fields at the end. Fortunately, all we do with the attrs struct after the call is to read a few specific fields out of it which are all at the same offsets in both structs, so we can simply make the local variable the correct type. This also lets us drop the cast (which should have been a bit of a warning flag that we were doing something wrong here). Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I don't know anything about the rdma code so this fix is based purely on looking at the code, and is untested beyond just make check/make check-avocado. --- hw/rdma/vmw/pvrdma_cmd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)