| Message ID | 20220719121110.225657-5-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | semihosting: fix various coverity issues | expand |
On 7/19/22 17:41, Peter Maydell wrote: > The TARGET_SYS_TMPNAM implementation has two bugs spotted by > Coverity: > * confusion about whether 'len' has the length of the string > including or excluding the terminating NUL means we > lock_user() len bytes of memory but memcpy() len + 1 bytes > * In the error-exit cases we forget to free() the buffer > that asprintf() returned to us > > Resolves: Coverity CID 1490285, 1490289 > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > semihosting/arm-compat-semi.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c index d12288fc806..e741674238f 100644 --- a/semihosting/arm-compat-semi.c +++ b/semihosting/arm-compat-semi.c @@ -504,16 +504,25 @@ void do_common_semihosting(CPUState *cs) GET_ARG(1); GET_ARG(2); len = asprintf(&s, "/tmp/qemu-%x%02x", getpid(), (int)arg1 & 0xff); + if (len < 0) { + common_semi_set_ret(cs, -1); + break; + } + + /* Allow for trailing NUL */ + len++; /* Make sure there's enough space in the buffer */ - if (len < 0 || len >= arg2) { + if (len > arg2) { + free(s); common_semi_set_ret(cs, -1); break; } p = lock_user(VERIFY_WRITE, arg0, len, 0); if (!p) { + free(s); goto do_fault; } - memcpy(p, s, len + 1); + memcpy(p, s, len); unlock_user(p, arg0, len); free(s); common_semi_set_ret(cs, 0);
The TARGET_SYS_TMPNAM implementation has two bugs spotted by Coverity: * confusion about whether 'len' has the length of the string including or excluding the terminating NUL means we lock_user() len bytes of memory but memcpy() len + 1 bytes * In the error-exit cases we forget to free() the buffer that asprintf() returned to us Resolves: Coverity CID 1490285, 1490289 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- semihosting/arm-compat-semi.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)