On Fri, 9 Jul 2021 at 06:17, David Gibson <da...@gibson.dropbear.id.au> wrote: > > From: Alexey Kardashevskiy <a...@ozlabs.ru> > > The PAPR platform describes an OS environment that's presented by > a combination of a hypervisor and firmware. The features it specifies > require collaboration between the firmware and the hypervisor.
Another Coverity issue unrelated to the others: CID 1458132: > +int vof_client_call(MachineState *ms, Vof *vof, void *fdt, > + target_ulong args_real) > +{ > + struct prom_args args_be; > + uint32_t args[ARRAY_SIZE(args_be.args)]; > + uint32_t rets[ARRAY_SIZE(args_be.args)] = { 0 }, ret; > + char service[64]; > + unsigned nargs, nret, i; > + > + if (VOF_MEM_READ(args_real, &args_be, sizeof(args_be)) != MEMTX_OK) { > + return -EINVAL; > + } > + nargs = be32_to_cpu(args_be.nargs); > + if (nargs >= ARRAY_SIZE(args_be.args)) { Our only bounds check against overflowing the args arrays is here, on 'nargs'... > + return -EINVAL; > + } > + > + if (VOF_MEM_READ(be32_to_cpu(args_be.service), service, sizeof(service)) > != > + MEMTX_OK) { > + return -EINVAL; > + } > + if (strnlen(service, sizeof(service)) == sizeof(service)) { > + /* Too long service name */ > + return -EINVAL; > + } > + > + for (i = 0; i < nargs; ++i) { > + args[i] = be32_to_cpu(args_be.args[i]); > + } > + > + nret = be32_to_cpu(args_be.nret); > + ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret); > + if (!nret) { > + return 0; > + } > + > + 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]); > + } ...but in the code we fill the args_be array with "nargs + 1 + nret - 1" values. Side note: is the code really intentionally copying only nret-1 values from rets[], or is the loop condition supposed to be "<= nret" ? > + > + if (VOF_MEM_WRITE(args_real + offsetof(struct prom_args, args[nargs]), > + args_be.args + nargs, sizeof(args_be.args[0]) * nret) > != > + MEMTX_OK) { > + return -EINVAL; > + } > + > + return 0; > +} thanks -- PMM