Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> [ Please pull from git://git.xenomai.org/xenomai-jki.git for-2.4.x ]
>>>
>>> The test for __xn_access_ok was inverted, thus rejected valid requests.
>>> Fix this and refactor the code in order to check for the actual size of
>>> the passed fd sets.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
>>> ---
>>>
>>>  ksrc/skins/posix/syscall.c |   21 +++++++++------------
>>>  1 files changed, 9 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/ksrc/skins/posix/syscall.c b/ksrc/skins/posix/syscall.c
>>> index 5c62a45..f0111f8 100644
>>> --- a/ksrc/skins/posix/syscall.c
>>> +++ b/ksrc/skins/posix/syscall.c
>>> @@ -2384,17 +2384,16 @@ static int __select(struct task_struct *curr, 
>>> struct pt_regs *regs)
>>>     struct timeval tv;
>>>     pthread_t thread;
>>>     int i, err, nfds;
>>> +   size_t fds_size;
>>>  
>>>     thread = pse51_current_thread();
>>>     if (!thread)
>>>             return -EPERM;
>>>  
>>>     if (__xn_reg_arg5(regs)) {
>>> -           if (__xn_access_ok(curr, VERIFY_WRITE,
>>> -                              __xn_reg_arg5(regs), sizeof(tv)))
>>> -                   return -EFAULT;
>>> -
>>> -           if (__xn_copy_from_user(curr, &tv,
>>> +           if (!__xn_access_ok(curr, VERIFY_WRITE,
>>> +                               __xn_reg_arg5(regs), sizeof(tv)) ||
>>> +               __xn_copy_from_user(curr, &tv,
>>>                                     (void __user *)__xn_reg_arg5(regs),
>>>                                     sizeof(tv)))
>>>                     return -EFAULT;
>> Ok.
>>
>>> @@ -2407,19 +2406,17 @@ static int __select(struct task_struct *curr, 
>>> struct pt_regs *regs)
>>>     }
>>>  
>>>     nfds = __xn_reg_arg1(regs);
>>> +   fds_size = __FDELT(nfds + __NFDBITS - 1) * sizeof(long);
>>>  
>>>     for (i = 0; i < XNSELECT_MAX_TYPES; i++)
>>>             if (ufd_sets[i]) {
>>>                     in_fds[i] = &in_fds_storage[i];
>>>                     out_fds[i] = & out_fds_storage[i];
>>> -                   if (__xn_access_ok(curr, VERIFY_WRITE,
>>> -                                      ufd_sets[i], sizeof(fd_set)))
>>> -                           return -EFAULT;
>>> -
>>> -                   if (__xn_copy_from_user(curr, in_fds[i],
>>> +                   if (!__xn_access_ok(curr, VERIFY_WRITE,
>>> +                                       ufd_sets[i], fds_size) ||
>> I do not think this is a good idea. There is no special mention to it in
>> select specification, but at least select prototype suggests that the
>> user-space structure should be a valid fd_set, not a partial subset of it.
> 
> Yeah, and we copy a full set back. Missed that, will update both patches.

This (and the corresponding head patch) is now available via git:

---------->

The test for __xn_access_ok was inverted, thus rejected valid requests.
Fix this and refactor the code in order to check for the actual size of
the passed fd sets.

Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
---

 ksrc/skins/posix/syscall.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/ksrc/skins/posix/syscall.c b/ksrc/skins/posix/syscall.c
index 5c62a45..3791071 100644
--- a/ksrc/skins/posix/syscall.c
+++ b/ksrc/skins/posix/syscall.c
@@ -2384,17 +2384,16 @@ static int __select(struct task_struct *curr, struct 
pt_regs *regs)
        struct timeval tv;
        pthread_t thread;
        int i, err, nfds;
+       size_t fds_size;
 
        thread = pse51_current_thread();
        if (!thread)
                return -EPERM;
 
        if (__xn_reg_arg5(regs)) {
-               if (__xn_access_ok(curr, VERIFY_WRITE,
-                                  __xn_reg_arg5(regs), sizeof(tv)))
-                       return -EFAULT;
-
-               if (__xn_copy_from_user(curr, &tv,
+               if (!__xn_access_ok(curr, VERIFY_WRITE,
+                                   __xn_reg_arg5(regs), sizeof(tv)) ||
+                   __xn_copy_from_user(curr, &tv,
                                        (void __user *)__xn_reg_arg5(regs),
                                        sizeof(tv)))
                        return -EFAULT;
@@ -2407,19 +2406,17 @@ static int __select(struct task_struct *curr, struct 
pt_regs *regs)
        }
 
        nfds = __xn_reg_arg1(regs);
+       fds_size = __FDELT(nfds + __NFDBITS - 1) * sizeof(long);
 
        for (i = 0; i < XNSELECT_MAX_TYPES; i++)
                if (ufd_sets[i]) {
                        in_fds[i] = &in_fds_storage[i];
                        out_fds[i] = & out_fds_storage[i];
-                       if (__xn_access_ok(curr, VERIFY_WRITE,
-                                          ufd_sets[i], sizeof(fd_set)))
-                               return -EFAULT;
-
-                       if (__xn_copy_from_user(curr, in_fds[i],
+                       if (!__xn_access_ok(curr, VERIFY_WRITE,
+                                           ufd_sets[i], sizeof(fd_set)) ||
+                           __xn_copy_from_user(curr, in_fds[i],
                                                (void __user *) ufd_sets[i],
-                                               __FDELT(nfds + __NFDBITS - 1)
-                                               * sizeof(long)))
+                                               fds_size))
                                return -EFAULT;
                }
 

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to