Re: [Qemu-devel] [PATCH 1/3] linux-user: Check sigsetsize argument to syscalls

[Peter Maydell]

On 21 June 2016 at 20:09, Laurent Vivier <address@hidden> wrote:
>
>
> Le 20/06/2016 à 16:50, Peter Maydell a écrit :
>> Many syscalls which take a sigset_t argument also take an argument
>> giving the size of the sigset_t.  The kernel insists that this
>> matches its idea of the type size and fails EINVAL if it is not.
>> Implement this logic in QEMU.  (This mostly just means some LTP test
>> cases which check error cases now pass.)
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  linux-user/syscall.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 95eafeb..7b3d129 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -7592,7 +7592,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>  #if defined(TARGET_ALPHA)
>>              struct target_sigaction act, oact, *pact = 0;
>>              struct target_rt_sigaction *rt_act;
>> -            /* ??? arg4 == sizeof(sigset_t).  */
>> +
>> +            if (arg4 != sizeof(target_sigset_t)) {
>> +                ret = -TARGET_EINVAL;
>> +                break;
>> +            }
>>              if (arg2) {
>>                  if (!lock_user_struct(VERIFY_READ, rt_act, arg2, 1))
>>                      goto efault;
>> @@ -7616,6 +7620,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>              struct target_sigaction *act;
>>              struct target_sigaction *oact;
>>
>> +            if (arg4 != sizeof(target_sigset_t)) {
>> +                ret = -TARGET_EINVAL;
>> +                break;
>> +            }
>>              if (arg2) {
>>                  if (!lock_user_struct(VERIFY_READ, act, arg2, 1))
>>                      goto efault;
>> @@ -7747,6 +7755,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>              int how = arg1;
>>              sigset_t set, oldset, *set_ptr;
>>
>> +            if (arg4 != sizeof(target_sigset_t)) {
>> +                ret = -TARGET_EINVAL;
>> +                goto fail;
>> +            }
>
> why "goto fail" instead of "break"?
> [you're not in a switch()]
>
>> +
>>              if (arg2) {
>>                  switch(how) {
>>                  case TARGET_SIG_BLOCK:
>> @@ -7797,6 +7810,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>      case TARGET_NR_rt_sigpending:
>>          {
>>              sigset_t set;
>> +
>> +            /* Yes, this check is >, not != like most. We follow the 
>> kernel's
>> +             * logic and it does it like this because it implements
>> +             * NR_sigpending through the same code path, and in that case
>> +             * the old_sigset_t is smaller in size.
>> +             */
>> +            if (arg2 > sizeof(target_sigset_t)) {
>> +                return -TARGET_EINVAL;
>> +            }
>
> why "return" instead of "break"?

Yeah, there's no particular reason for these inconsistencies,
so I guess we should just use 'break' everywhere.

thanks
-- PMM