* [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select @ 2009-05-07 19:25 Jan Kiszka 2009-05-07 22:50 ` Gilles Chanteperdrix 0 siblings, 1 reply; 4+ messages in thread From: Jan Kiszka @ 2009-05-07 19:25 UTC (permalink / raw) To: Philippe Gerum; +Cc: Vladimir Zapolskiy, xenomai-core [-- Attachment #1: Type: text/plain, Size: 2000 bytes --] [ 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.kiszka@domain.hid> --- 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; @@ -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) || + __xn_copy_from_user(curr, in_fds[i], (void __user *) ufd_sets[i], - __FDELT(nfds + __NFDBITS - 1) - * sizeof(long))) + fds_size)) return -EFAULT; } [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select 2009-05-07 19:25 [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select Jan Kiszka @ 2009-05-07 22:50 ` Gilles Chanteperdrix 2009-05-07 23:17 ` Jan Kiszka 0 siblings, 1 reply; 4+ messages in thread From: Gilles Chanteperdrix @ 2009-05-07 22:50 UTC (permalink / raw) To: Jan Kiszka; +Cc: Vladimir Zapolskiy, xenomai-core 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.kiszka@domain.hid> > --- > > 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. > + __xn_copy_from_user(curr, in_fds[i], > (void __user *) ufd_sets[i], > - __FDELT(nfds + __NFDBITS - 1) > - * sizeof(long))) > + fds_size)) > return -EFAULT; > } > > -- Gilles. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select 2009-05-07 22:50 ` Gilles Chanteperdrix @ 2009-05-07 23:17 ` Jan Kiszka 2009-05-07 23:24 ` Jan Kiszka 0 siblings, 1 reply; 4+ messages in thread From: Jan Kiszka @ 2009-05-07 23:17 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Vladimir Zapolskiy, xenomai-core [-- Attachment #1: Type: text/plain, Size: 2345 bytes --] 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.kiszka@domain.hid> >> --- >> >> 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. Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select 2009-05-07 23:17 ` Jan Kiszka @ 2009-05-07 23:24 ` Jan Kiszka 0 siblings, 0 replies; 4+ messages in thread From: Jan Kiszka @ 2009-05-07 23:24 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Vladimir Zapolskiy, xenomai-core [-- Attachment #1: Type: text/plain, Size: 4433 bytes --] 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.kiszka@domain.hid> >>> --- >>> >>> 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.kiszka@domain.hid> --- 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 #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-05-07 23:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-05-07 19:25 [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select Jan Kiszka 2009-05-07 22:50 ` Gilles Chanteperdrix 2009-05-07 23:17 ` Jan Kiszka 2009-05-07 23:24 ` Jan Kiszka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.