Re: [Qemu-arm] [PULL 06/28] target/arm: use the common interface for WRITE0/WRITEC in arm-semi

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On Tue, 28 May 2019 at 10:49, Alex Bennée <address@hidden> wrote:
>>
>> Now we have a common semihosting console interface use that for our
>> string output. However ARM is currently unique in also supporting
>> semihosting for linux-user so we need to replicate the API in
>> linux-user. If other architectures gain this support we can move the
>> file later.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>> Reviewed-by: Richard Henderson <address@hidden>
>
> Hi; Coverity points out an issue in this function (CID 1401700):
>
>
>> +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int 
>> len)
>> +{
>> +    void *s = lock_user_string(addr);
>> +    len = write(STDERR_FILENO, s, len ? len : strlen(s));
>> +    unlock_user(s, addr, 0);
>> +    return len;
>> +}
>
> We call lock_user_string(), which can fail and return NULL
> if the memory pointed to by addr isn't actually readable.
> But we don't check for the error, so we can pass a NULL
> pointer to write().

Mea culpa - I'd avoided the nastiness of the lock string stuff in the
softmmu case but reverted to a naive implementation for linux-user after
the fact. I'll send a fix for that.

> Also it looks a bit dodgy that we are passed in a
> specific length value but we then go and look at the length
> of the string, but we trust the specific length value over
> the length of the string. If len is larger than the real
> length of the string (including terminating NUL) then the
> write() will read off the end of the string.

It is an admittedly in-elegant hack to deal with the fact we call the
same function for outputting a character as well as a string. None of
the guests actually give us the length:

 * @len: length of string or 0 (string is null terminated)

We could formalise it by making s/len/is_char/ and making it a bool or
just add some more text to the description.

>
> thanks
> -- PMM


--
Alex Bennée