Re: [PULL 18/40] linux-user: Fix types in uaccess.c

[Richard Henderson <richard.henderson@linaro.org>]

On 3/10/21 10:34 AM, Laurent Vivier wrote:
> Le 10/03/2021 à 16:48, Peter Maydell a écrit :
>> On Fri, 19 Feb 2021 at 09:21, Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>> Hi Richard,
>>>
>>> I think this commit is the cause of CID 1446711.
>>>
>>> There is no concistancy between the various declarations of unlock_user():
>>>
>>> bsd-user/qemu.h
>>>
>>> static inline void unlock_user(void *host_ptr, abi_ulong guest_addr,
>>>                                 long len)
>>>
>>> include/exec/softmmu-semi.h
>>>
>>> static void softmmu_unlock_user(CPUArchState *env, void *p, target_ulong addr,
>>>                                  target_ulong len)
>>> ...
>>> #define unlock_user(s, args, len) softmmu_unlock_user(env, s, args, len)
>>>
>>> linux-user/qemu.h
>>>
>>> #ifndef DEBUG_REMAP
>>> static inline void unlock_user(void *host_ptr, abi_ulong guest_addr, size_t len)
>>> { }
>>> #else
>>> void unlock_user(void *host_ptr, abi_ulong guest_addr, long len);
>>> #endif
>>>
>>> To take a signed long here allows to unconditionnaly call the unlock_user() function after the
>>> syscall and not to copy the buffer if the value is negative.
>>
>> Hi; what was the conclusion here about how best to fix the Coverity issue?
> 
> For what I know, there is no conclusion.
> 
>> To save people looking it up, Coverity complains because in the
>> TARGET_NR_readlinkat case for linux-user we do:
>>              void *p2;
>>              p  = lock_user_string(arg2);
>>              p2 = lock_user(VERIFY_WRITE, arg3, arg4, 0);
>>              if (!p || !p2) {
>>                  ret = -TARGET_EFAULT;
>>              } else if (is_proc_myself((const char *)p, "exe")) {
>>                  char real[PATH_MAX], *temp;
>>                  temp = realpath(exec_path, real);
>>                  ret = temp == NULL ? get_errno(-1) : strlen(real) ;
>>                  snprintf((char *)p2, arg4, "%s", real);
>>              } else {
>>                  ret = get_errno(readlinkat(arg1, path(p), p2, arg4));
>>              }
>>              unlock_user(p2, arg3, ret);
>>              unlock_user(p, arg2, 0);
>>
>> and in the "ret = -TARGET_EFAULT" and also the get_errno(readlinkat(...))
>> codepaths we can set ret to a negative number, which Coverity thinks
>> is suspicious given that unlock_user()'s new prototype says it
>> is an unsigned value. It's correct to be suspicious, because we really
>> did change from doing a >=0 to a !=0 check on the length.
>>
>> Unless we really want to audit all the unlock_user() callsites,
>> going back to the previous semantics seems sensible.
> 
> I agree with that.

This passing of an errno to unlock_user is... horrible.  I guess we have to 
revert to the signed type.


r~