| Message ID | 20210713134638.1803316-1-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [qemu] ppc/vof: Fix Coverity issues | expand |
On Tue, Jul 13, 2021 at 11:46:38PM +1000, Alexey Kardashevskiy wrote: > This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity. > > This adds a comment about the return parameters number in the VOF hcall. > The reason for such counting is to keep the numbers look the same in > vof_client_handle() and the Linux (an OF client). > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > Will this make COverity happy? What is the canonical way of fixing these > uint32_t vs. int? Thanks, It might make Coverity happy, but I think it's an ugly approach. > > --- > hw/ppc/vof.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c > index 81f65962156c..872f671babbe 100644 > --- a/hw/ppc/vof.c > +++ b/hw/ppc/vof.c > @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle) > static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, > uint32_t buf, uint32_t len) > { > - uint32_t ret = -1; > + int ret = -1; I don't think you want to try to use the same variable for the value from phandle_to_path() and the return value from this function - they're different types, with different encodings. The inner value should remain int (that's the libfdt convention). The outer one is explicltly unsigned. You're not really looking for negative error values, but specifically for -1U == ~0U as the single error value. So re-introduce your PROM_ERROR valued, defined as ~0U, so that it's clearly unsigned, and use that and unsigned logic for all manipulation of the outer value. > char tmp[VOF_MAX_PATH] = ""; > > ret = phandle_to_path(fdt, phandle, tmp, sizeof(tmp)); > @@ -529,13 +529,13 @@ static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, > > trace_vof_package_to_path(phandle, tmp, ret); > > - return ret; > + return (uint32_t) ret; > } > > static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle, > uint32_t buf, uint32_t len) > { > - uint32_t ret = -1; > + int ret = -1; > uint32_t phandle = vof_instance_to_package(vof, ihandle); > char tmp[VOF_MAX_PATH] = ""; > > @@ -549,7 +549,7 @@ static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle, > } > trace_vof_instance_to_path(ihandle, phandle, tmp, ret); > > - return ret; > + return (uint32_t) ret; > } > > static uint32_t vof_write(Vof *vof, uint32_t ihandle, uint32_t buf, > @@ -965,11 +965,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void *fdt, > } > > nret = be32_to_cpu(args_be.nret); > + if (nret > ARRAY_SIZE(args_be.args) - nargs) { > + return -EINVAL; > + } That looks reasonable. > ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret); > if (!nret) { > return 0; > } > > + /* @nrets includes the value which this function returns */ > args_be.args[nargs] = cpu_to_be32(ret); > for (i = 1; i < nret; ++i) { > args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);
On Tue, 13 Jul 2021 23:46:38 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity. > > This adds a comment about the return parameters number in the VOF hcall. > The reason for such counting is to keep the numbers look the same in > vof_client_handle() and the Linux (an OF client). > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > Will this make COverity happy? What is the canonical way of fixing these > uint32_t vs. int? Thanks, > You might want to mention the Coverity IDs fixed by this patch in the changelog, e.g. Fixes: CID xxxxxxx, yyyyyyy > --- > hw/ppc/vof.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c > index 81f65962156c..872f671babbe 100644 > --- a/hw/ppc/vof.c > +++ b/hw/ppc/vof.c > @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle) > static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, > uint32_t buf, uint32_t len) > { > - uint32_t ret = -1; > + int ret = -1; > char tmp[VOF_MAX_PATH] = ""; > > ret = phandle_to_path(fdt, phandle, tmp, sizeof(tmp)); > @@ -529,13 +529,13 @@ static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, > > trace_vof_package_to_path(phandle, tmp, ret); > > - return ret; > + return (uint32_t) ret; > } > > static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle, > uint32_t buf, uint32_t len) > { > - uint32_t ret = -1; > + int ret = -1; > uint32_t phandle = vof_instance_to_package(vof, ihandle); > char tmp[VOF_MAX_PATH] = ""; > > @@ -549,7 +549,7 @@ static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle, > } > trace_vof_instance_to_path(ihandle, phandle, tmp, ret); > > - return ret; > + return (uint32_t) ret; > } > > static uint32_t vof_write(Vof *vof, uint32_t ihandle, uint32_t buf, > @@ -965,11 +965,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void *fdt, > } > > nret = be32_to_cpu(args_be.nret); > + if (nret > ARRAY_SIZE(args_be.args) - nargs) { > + return -EINVAL; > + } > ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret); > if (!nret) { > return 0; > } > > + /* @nrets includes the value which this function returns */ > args_be.args[nargs] = cpu_to_be32(ret); > for (i = 1; i < nret; ++i) { > args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);
On 7/19/21 13:57, David Gibson wrote: > On Tue, Jul 13, 2021 at 11:46:38PM +1000, Alexey Kardashevskiy wrote: >> This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity. >> >> This adds a comment about the return parameters number in the VOF hcall. >> The reason for such counting is to keep the numbers look the same in >> vof_client_handle() and the Linux (an OF client). >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> >> Will this make COverity happy? What is the canonical way of fixing these >> uint32_t vs. int? Thanks, > > It might make Coverity happy, but I think it's an ugly approach. > >> >> --- >> hw/ppc/vof.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c >> index 81f65962156c..872f671babbe 100644 >> --- a/hw/ppc/vof.c >> +++ b/hw/ppc/vof.c >> @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle) >> static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, >> uint32_t buf, uint32_t len) >> { >> - uint32_t ret = -1; >> + int ret = -1; > > I don't think you want to try to use the same variable for the value > from phandle_to_path() and the return value from this function - > they're different types, with different encodings. The inner value > should remain int (that's the libfdt convention). > > The outer one is explicltly unsigned. You're not really looking for > negative error values, but specifically for -1U == ~0U as the single > error value. So re-introduce your PROM_ERROR valued, defined as ~0U, > so that it's clearly unsigned, and use that and unsigned logic for all > manipulation of the outer value. Fair enough. One question. Linux defines it as #define PROM_ERROR (-1u) Do you still vote for "~0U"?
On Mon, Jul 19, 2021 at 06:25:53PM +1000, Alexey Kardashevskiy wrote: > > > On 7/19/21 13:57, David Gibson wrote: > > On Tue, Jul 13, 2021 at 11:46:38PM +1000, Alexey Kardashevskiy wrote: > > > This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity. > > > > > > This adds a comment about the return parameters number in the VOF hcall. > > > The reason for such counting is to keep the numbers look the same in > > > vof_client_handle() and the Linux (an OF client). > > > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > --- > > > > > > Will this make COverity happy? What is the canonical way of fixing these > > > uint32_t vs. int? Thanks, > > > > It might make Coverity happy, but I think it's an ugly approach. > > > > > > > > --- > > > hw/ppc/vof.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c > > > index 81f65962156c..872f671babbe 100644 > > > --- a/hw/ppc/vof.c > > > +++ b/hw/ppc/vof.c > > > @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle) > > > static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, > > > uint32_t buf, uint32_t len) > > > { > > > - uint32_t ret = -1; > > > + int ret = -1; > > > > I don't think you want to try to use the same variable for the value > > from phandle_to_path() and the return value from this function - > > they're different types, with different encodings. The inner value > > should remain int (that's the libfdt convention). > > > > The outer one is explicltly unsigned. You're not really looking for > > negative error values, but specifically for -1U == ~0U as the single > > error value. So re-introduce your PROM_ERROR valued, defined as ~0U, > > so that it's clearly unsigned, and use that and unsigned logic for all > > manipulation of the outer value. > > > Fair enough. One question. Linux defines it as > > #define PROM_ERROR (-1u) > > Do you still vote for "~0U"? I don't really mind. I think (-1U) might cause some more Coverity confusion that ~0U, based on experience with Coverity scans of dtc & libfdt.
diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c index 81f65962156c..872f671babbe 100644 --- a/hw/ppc/vof.c +++ b/hw/ppc/vof.c @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle) static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, uint32_t buf, uint32_t len) { - uint32_t ret = -1; + int ret = -1; char tmp[VOF_MAX_PATH] = ""; ret = phandle_to_path(fdt, phandle, tmp, sizeof(tmp)); @@ -529,13 +529,13 @@ static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle, trace_vof_package_to_path(phandle, tmp, ret); - return ret; + return (uint32_t) ret; } static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle, uint32_t buf, uint32_t len) { - uint32_t ret = -1; + int ret = -1; uint32_t phandle = vof_instance_to_package(vof, ihandle); char tmp[VOF_MAX_PATH] = ""; @@ -549,7 +549,7 @@ static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle, } trace_vof_instance_to_path(ihandle, phandle, tmp, ret); - return ret; + return (uint32_t) ret; } static uint32_t vof_write(Vof *vof, uint32_t ihandle, uint32_t buf, @@ -965,11 +965,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void *fdt, } nret = be32_to_cpu(args_be.nret); + if (nret > ARRAY_SIZE(args_be.args) - nargs) { + return -EINVAL; + } ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret); if (!nret) { return 0; } + /* @nrets includes the value which this function returns */ args_be.args[nargs] = cpu_to_be32(ret); for (i = 1; i < nret; ++i) { args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);
This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity. This adds a comment about the return parameters number in the VOF hcall. The reason for such counting is to keep the numbers look the same in vof_client_handle() and the Linux (an OF client). Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Will this make COverity happy? What is the canonical way of fixing these uint32_t vs. int? Thanks, --- hw/ppc/vof.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)