Re: [PATCH qemu v2] ppc/vof: Fix Coverity issues
On Tue, Jul 20, 2021 at 03:07:26PM +1000, Alexey Kardashevskiy wrote:
> Coverity reported issues which are caused by mixing of signed return codes
> from DTC and unsigned return codes of the client interface.
>
> This introduces PROM_ERROR and makes distinction between the error types.
>
> This fixes NEGATIVE_RETURNS, OVERRUN issues reported by 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).
>
> vmc->client_architecture_support() returns target_ulong and we want to
> propagate this to the client (for example H_MULTI_THREADS_ACTIVE).
> The VOF path to do_client_architecture_support() needs chopping off
> the top 32bit but SLOF's H_CAS does not; and either way the return values
> are either 0 or 32bit negative error code. For now this chops
> the top 32bits.
>
> This makes "claim" fail if the allocated address is above 4GB as
> the client interface is 32bit. This still allows claiming memory above
> 4GB as potentially initrd can be put there and the client can read
> the address from the FDT's "available" property.
>
> Fixes: CID 1458139, 1458138, 1458137, 1458133, 1458132
> Signed-off-by: Alexey Kardashevskiy
Well, I don't know for sure if it will fix every one, but it certainly
looks like an improvement. So, applied to ppc-for-6.1.
Couple of minor notes:
[snip]
> @@ -461,18 +458,18 @@ trace_exit:
> uint32_t vof_client_open_store(void *fdt, Vof *vof, const char *nodename,
> const char *prop, const char *path)
> {
> -int node = fdt_path_offset(fdt, nodename);
> -int inst, offset;
> +int offset, node = fdt_path_offset(fdt, nodename);
> +uint32_t inst;
>
> offset = fdt_path_offset(fdt, path);
> if (offset < 0) {
> trace_vof_error_unknown_path(path);
> -return offset;
> +return PROM_ERROR;
Note that this case was a real bug, not just Coverity being pedantic.
> }
>
> inst = vof_do_open(fdt, vof, offset, path);
>
> -return fdt_setprop_cell(fdt, node, prop, inst);
> +return fdt_setprop_cell(fdt, node, prop, inst) >= 0 ? 0 : PROM_ERROR;
> }
>
> static uint32_t vof_open(void *fdt, Vof *vof, uint32_t pathaddr)
> @@ -481,13 +478,13 @@ static uint32_t vof_open(void *fdt, Vof *vof, uint32_t
> pathaddr)
> int offset;
>
> if (readstr(pathaddr, path, sizeof(path))) {
> -return -1;
> +return PROM_ERROR;
> }
>
> offset = path_offset(fdt, path);
> if (offset < 0) {
> trace_vof_error_unknown_path(path);
> -return offset;
> +return PROM_ERROR;
As is this one.
[snip]
> @@ -826,7 +824,7 @@ trace_exit:
> static uint32_t vof_call_interpret(uint32_t cmdaddr, uint32_t param1,
> uint32_t param2, uint32_t *ret2)
> {
> -uint32_t ret = -1;
> +uint32_t ret = PROM_ERROR;
> char cmd[VOF_MAX_FORTHCODE] = "";
>
> /* No interpret implemented so just call a trace */
> @@ -895,13 +893,20 @@ static uint32_t vof_client_handle(MachineState *ms,
> void *fdt, Vof *vof,
> } else if (cmpserv("write", 3, 1)) {
> ret = vof_write(vof, args[0], args[1], args[2]);
> } else if (cmpserv("claim", 3, 1)) {
> -ret = vof_claim(vof, args[0], args[1], args[2]);
> -if (ret != -1) {
> +uint64_t ret64 = vof_claim(vof, args[0], args[1], args[2]);
> +
> +if (ret64 < 0x1UL) {
This assumes that 2^32 fits in an unsigned long; I'm not certainly if
that's true for all supported qemu host arches. ULL would be safer
here.
> vof_dt_memory_available(fdt, vof->claimed, vof->claimed_base);
> +ret = (uint32_t)ret64;
> +} else {
> +if (ret64 != -1) {
> +vof_release(vof, ret, args[1]);
> +}
> +ret = PROM_ERROR;
> }
> } else if (cmpserv("release", 2, 0)) {
> ret = vof_release(vof, args[0], args[1]);
> -if (ret != -1) {
> +if (ret != PROM_ERROR) {
> vof_dt_memory_available(fdt, vof->claimed, vof->claimed_base);
> }
> } else if (cmpserv("call-method", 0, 0)) {
> @@ -965,11 +970,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]);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 6e90a0107247..da6e74b80dc1 100644
> --- a/hw/ppc/trace-events
> +++
[PATCH qemu v2] ppc/vof: Fix Coverity issues
Coverity reported issues which are caused by mixing of signed return codes
from DTC and unsigned return codes of the client interface.
This introduces PROM_ERROR and makes distinction between the error types.
This fixes NEGATIVE_RETURNS, OVERRUN issues reported by 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).
vmc->client_architecture_support() returns target_ulong and we want to
propagate this to the client (for example H_MULTI_THREADS_ACTIVE).
The VOF path to do_client_architecture_support() needs chopping off
the top 32bit but SLOF's H_CAS does not; and either way the return values
are either 0 or 32bit negative error code. For now this chops
the top 32bits.
This makes "claim" fail if the allocated address is above 4GB as
the client interface is 32bit. This still allows claiming memory above
4GB as potentially initrd can be put there and the client can read
the address from the FDT's "available" property.
Fixes: CID 1458139, 1458138, 1458137, 1458133, 1458132
Signed-off-by: Alexey Kardashevskiy
---
Changes:
v2:
* fixed signed/unsigned errors convertion to make error types more explicit
---
include/hw/ppc/vof.h | 2 +
hw/ppc/vof.c | 139 +++
hw/ppc/trace-events | 4 +-
3 files changed, 78 insertions(+), 67 deletions(-)
diff --git a/include/hw/ppc/vof.h b/include/hw/ppc/vof.h
index 640be46163cb..97fdef758bf2 100644
--- a/include/hw/ppc/vof.h
+++ b/include/hw/ppc/vof.h
@@ -55,4 +55,6 @@ struct VofMachineIfClass {
address_space_write(_space_memory, \
(pa), MEMTXATTRS_UNSPECIFIED, (buf), (size))
+#define PROM_ERROR (~0U)
+
#endif /* HW_VOF_H */
diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index 81f65962156c..73adc44ec212 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -160,7 +160,7 @@ static int path_offset(const void *fdt, const char *path)
static uint32_t vof_finddevice(const void *fdt, uint32_t nodeaddr)
{
char fullnode[VOF_MAX_PATH];
-uint32_t ret = -1;
+uint32_t ret = PROM_ERROR;
int offset;
if (readstr(nodeaddr, fullnode, sizeof(fullnode))) {
@@ -172,7 +172,7 @@ static uint32_t vof_finddevice(const void *fdt, uint32_t
nodeaddr)
ret = fdt_get_phandle(fdt, offset);
}
trace_vof_finddevice(fullnode, ret);
-return (uint32_t) ret;
+return ret;
}
static const void *getprop(const void *fdt, int nodeoff, const char *propname,
@@ -229,10 +229,10 @@ static uint32_t vof_getprop(const void *fdt, uint32_t
nodeph, uint32_t pname,
bool write0;
if (nodeoff < 0) {
-return -1;
+return PROM_ERROR;
}
if (readstr(pname, propname, sizeof(propname))) {
-return -1;
+return PROM_ERROR;
}
prop = getprop(fdt, nodeoff, propname, , );
if (prop) {
@@ -244,7 +244,7 @@ static uint32_t vof_getprop(const void *fdt, uint32_t
nodeph, uint32_t pname,
(write0 &&
cb == proplen &&
VOF_MEM_WRITE(valaddr + cb - 1, , 1) != MEMTX_OK)) {
-ret = -1;
+ret = PROM_ERROR;
} else {
/*
* OF1275 says:
@@ -259,7 +259,7 @@ static uint32_t vof_getprop(const void *fdt, uint32_t
nodeph, uint32_t pname,
}
}
} else {
-ret = -1;
+ret = PROM_ERROR;
}
trace_vof_getprop(nodeph, propname, ret, trval);
@@ -275,16 +275,16 @@ static uint32_t vof_getproplen(const void *fdt, uint32_t
nodeph, uint32_t pname)
int nodeoff = fdt_node_offset_by_phandle(fdt, nodeph);
if (nodeoff < 0) {
-return -1;
+return PROM_ERROR;
}
if (readstr(pname, propname, sizeof(propname))) {
-return -1;
+return PROM_ERROR;
}
prop = getprop(fdt, nodeoff, propname, , NULL);
if (prop) {
ret = proplen;
} else {
-ret = -1;
+ret = PROM_ERROR;
}
trace_vof_getproplen(nodeph, propname, ret);
@@ -296,8 +296,8 @@ static uint32_t vof_setprop(MachineState *ms, void *fdt,
Vof *vof,
uint32_t valaddr, uint32_t vallen)
{
char propname[OF_PROPNAME_LEN_MAX + 1];
-uint32_t ret = -1;
-int offset;
+uint32_t ret = PROM_ERROR;
+int offset, rc;
char trval[64] = "";
char nodepath[VOF_MAX_PATH] = "";
Object *vmo = object_dynamic_cast(OBJECT(ms), TYPE_VOF_MACHINE_IF);
@@ -314,8 +314,8 @@ static uint32_t vof_setprop(MachineState *ms, void *fdt,
Vof *vof,
if (offset < 0) {
goto trace_exit;
}
-ret = get_path(fdt, offset, nodepath, sizeof(nodepath));
-if (ret <= 0) {
+rc = get_path(fdt, offset, nodepath, sizeof(nodepath));
+if (rc <= 0) {
goto trace_exit;
}
@@ -333,8 +333,8 @@ static uint32_t vof_setprop(MachineState *ms, void *fdt,
Vof *vof,
goto trace_exit;
}
-ret =
