diff mbox

[1/3] linux-user: Check sigsetsize argument to syscalls

Message ID 1466434237-19334-2-git-send-email-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Maydell June 20, 2016, 2:50 p.m. UTC 
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 <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

Comments

Laurent Vivier June 21, 2016, 7:09 p.m. UTC | #1 
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 <peter.maydell@linaro.org>
> ---
>  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"?

> +
>              ret = get_errno(sigpending(&set));
>              if (!is_error(ret)) {
>                  if (!(p = lock_user(VERIFY_WRITE, arg1, sizeof(target_sigset_t), 0)))
> @@ -7830,6 +7853,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_rt_sigsuspend:
>          {
>              TaskState *ts = cpu->opaque;
> +
> +            if (arg2 != sizeof(target_sigset_t)) {
> +                ret = -TARGET_EINVAL;
> +                break;
> +            }
>              if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
>                  goto efault;
>              target_to_host_sigset(&ts->sigsuspend_mask, p);
> @@ -7847,6 +7875,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              struct timespec uts, *puts;
>              siginfo_t uinfo;
>  
> +            if (arg4 != sizeof(target_sigset_t)) {
> +                ret = -TARGET_EINVAL;
> +                break;
> +            }
> +
>              if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
>                  goto efault;
>              target_to_host_sigset(&set, p);
> @@ -9095,6 +9128,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  }
>  
>                  if (arg4) {
> +                    if (arg5 != sizeof(target_sigset_t)) {
> +                        unlock_user(target_pfd, arg1, 0);
> +                        ret = -TARGET_EINVAL;
> +                        break;
> +                    }
> +
>                      target_set = lock_user(VERIFY_READ, arg4, sizeof(target_sigset_t), 1);
>                      if (!target_set) {
>                          unlock_user(target_pfd, arg1, 0);
> @@ -10914,6 +10953,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              sigset_t _set, *set = &_set;
>  
>              if (arg5) {
> +                if (arg6 != sizeof(target_sigset_t)) {
> +                    ret = -TARGET_EINVAL;
> +                    break;
> +                }
> +
>                  target_set = lock_user(VERIFY_READ, arg5,
>                                         sizeof(target_sigset_t), 1);
>                  if (!target_set) {
>
Peter Maydell June 21, 2016, 7:50 p.m. UTC | #2 
On 21 June 2016 at 20:09, Laurent Vivier <laurent@vivier.eu> 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 <peter.maydell@linaro.org>
>> ---
>>  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
diff mbox

Patch

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;
+            }
+
             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;
+            }
+
             ret = get_errno(sigpending(&set));
             if (!is_error(ret)) {
                 if (!(p = lock_user(VERIFY_WRITE, arg1, sizeof(target_sigset_t), 0)))
@@ -7830,6 +7853,11 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_rt_sigsuspend:
         {
             TaskState *ts = cpu->opaque;
+
+            if (arg2 != sizeof(target_sigset_t)) {
+                ret = -TARGET_EINVAL;
+                break;
+            }
             if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
                 goto efault;
             target_to_host_sigset(&ts->sigsuspend_mask, p);
@@ -7847,6 +7875,11 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             struct timespec uts, *puts;
             siginfo_t uinfo;
 
+            if (arg4 != sizeof(target_sigset_t)) {
+                ret = -TARGET_EINVAL;
+                break;
+            }
+
             if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
                 goto efault;
             target_to_host_sigset(&set, p);
@@ -9095,6 +9128,12 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 }
 
                 if (arg4) {
+                    if (arg5 != sizeof(target_sigset_t)) {
+                        unlock_user(target_pfd, arg1, 0);
+                        ret = -TARGET_EINVAL;
+                        break;
+                    }
+
                     target_set = lock_user(VERIFY_READ, arg4, sizeof(target_sigset_t), 1);
                     if (!target_set) {
                         unlock_user(target_pfd, arg1, 0);
@@ -10914,6 +10953,11 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             sigset_t _set, *set = &_set;
 
             if (arg5) {
+                if (arg6 != sizeof(target_sigset_t)) {
+                    ret = -TARGET_EINVAL;
+                    break;
+                }
+
                 target_set = lock_user(VERIFY_READ, arg5,
                                        sizeof(target_sigset_t), 1);
                 if (!target_set) {