[Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select

[Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select

[Jan Kiszka]

[-- 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 --]

Re: [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select

[Gilles Chanteperdrix]

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.

Re: [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select

[Jan Kiszka]

[-- 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 --]

Re: [Xenomai-core] [PATCH][STABLE] posix: Fix access checks in select

[Jan Kiszka]

[-- 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 --]