[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 18/40] linux-user: Fix types in uaccess.c
From: |
Laurent Vivier |
Subject: |
Re: [PULL 18/40] linux-user: Fix types in uaccess.c |
Date: |
Wed, 10 Mar 2021 17:34:34 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 |
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.
Thanks,
Laurent