[Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

Obviously a conversion error while switching to __xn_safe*.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/skins/posix/syscall.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ksrc/skins/posix/syscall.c b/ksrc/skins/posix/syscall.c
index 16303b3..0eca3d0 100644
--- a/ksrc/skins/posix/syscall.c
+++ b/ksrc/skins/posix/syscall.c
@@ -1663,7 +1663,7 @@ static int __mq_open(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))
@@ -1736,7 +1736,7 @@ static int __mq_unlink(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))
@@ -2440,7 +2440,7 @@ static int __shm_open(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))
@@ -2487,7 +2487,7 @@ static int __shm_unlink(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Obviously a conversion error while switching to __xn_safe*.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

Well, I have just checked the kernel code, and 0 as a return value of
strncpy_from_user is treated as a value in most places, even if not -EFAULT.

-- 
                                                 Gilles.

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Obviously a conversion error while switching to __xn_safe*.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
> Well, I have just checked the kernel code, and 0 as a return value of
> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
> 

Better check our code: :) __xn_safe_strncpy_from_user works differently.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Obviously a conversion error while switching to __xn_safe*.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> Well, I have just checked the kernel code, and 0 as a return value of
>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>
> 
> Better check our code: :) __xn_safe_strncpy_from_user works differently.

Then I would tend to consider that xn_safe_strncpy is broken.

-- 
                                                 Gilles.

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Obviously a conversion error while switching to __xn_safe*.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> Well, I have just checked the kernel code, and 0 as a return value of
>>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>>
>> Better check our code: :) __xn_safe_strncpy_from_user works differently.
> 
> Then I would tend to consider that xn_safe_strncpy is broken.

No, because it not a derivate of strncpy_from_user, but an internal
service optimized for the most common use cases (where you don't care
about the precise return value).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Obviously a conversion error while switching to __xn_safe*.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Well, I have just checked the kernel code, and 0 as a return value of
>>>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>>>
>>> Better check our code: :) __xn_safe_strncpy_from_user works differently.
>> Then I would tend to consider that xn_safe_strncpy is broken.
> 
> No, because it not a derivate of strncpy_from_user, but an internal
> service optimized for the most common use cases (where you don't care
> about the precise return value).

So, what should I call if I care about the return value ?

-- 
                                                 Gilles.

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Obviously a conversion error while switching to __xn_safe*.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Well, I have just checked the kernel code, and 0 as a return value of
>>>>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>>>>
>>>> Better check our code: :) __xn_safe_strncpy_from_user works differently.
>>> Then I would tend to consider that xn_safe_strncpy is broken.
>> No, because it not a derivate of strncpy_from_user, but an internal
>> service optimized for the most common use cases (where you don't care
>> about the precise return value).
> 
> So, what should I call if I care about the return value ?

The old combo of access_rok() and __xn_strncpy_from_user() - ah, I see
the issue: POSIX requires the length to report overflows to the users.
Hmm, then back to the old code, just adding the missing address range check.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Obviously a conversion error while switching to __xn_safe*.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>> Well, I have just checked the kernel code, and 0 as a return value of
>>>>>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>>>>>
>>>>> Better check our code: :) __xn_safe_strncpy_from_user works differently.
>>>> Then I would tend to consider that xn_safe_strncpy is broken.
>>> No, because it not a derivate of strncpy_from_user, but an internal
>>> service optimized for the most common use cases (where you don't care
>>> about the precise return value).
>> So, what should I call if I care about the return value ?
> 
> The old combo of access_rok() and __xn_strncpy_from_user() - ah, I see
> the issue: POSIX requires the length to report overflows to the users.
> Hmm, then back to the old code, just adding the missing address range check.
> 

Forget that, now I should have looked into our code: The "safe" version
transparently calls __xn_strncpy_from_user, no need to go by foot here.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2681 bytes --]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Obviously a conversion error while switching to __xn_safe*.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>> Well, I have just checked the kernel code, and 0 as a return value of
>>>>>>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>>>>>>
>>>>>> Better check our code: :) __xn_safe_strncpy_from_user works differently.
>>>>> Then I would tend to consider that xn_safe_strncpy is broken.
>>>> No, because it not a derivate of strncpy_from_user, but an internal
>>>> service optimized for the most common use cases (where you don't care
>>>> about the precise return value).
>>> So, what should I call if I care about the return value ?
>> The old combo of access_rok() and __xn_strncpy_from_user() - ah, I see
>> the issue: POSIX requires the length to report overflows to the users.
>> Hmm, then back to the old code, just adding the missing address range check.
>>
> 
> Forget that, now I should have looked into our code: The "safe" version
> transparently calls __xn_strncpy_from_user, no need to go by foot here.
> 

Here is the real bug:

>         long res;
>         __do_strncpy_from_user(dst, src, count, res);
> ffffffff804505cc:       48 85 c9                test   %rcx,%rcx
> ffffffff804505cf:       74 0e                   je     ffffffff804505df <rthal_strncpy_from_user+0x1f>
> ffffffff804505d1:       ac                      lods   %ds:(%rsi),%al
> ffffffff804505d2:       aa                      stos   %al,%es:(%rdi)
> ffffffff804505d3:       84 c0                   test   %al,%al
> ffffffff804505d5:       74 05                   je     ffffffff804505dc <rthal_strncpy_from_user+0x1c>
> ffffffff804505d7:       48 ff c9                dec    %rcx
> ffffffff804505da:       75 f5                   jne    ffffffff804505d1 <rthal_strncpy_from_user+0x11>
> ffffffff804505dc:       48 29 c9                sub    %rcx,%rcx
                                                         ^^^^^^^^^

>         return res;
> }
> ffffffff804505df:       48 89 c8                mov    %rcx,%rax
> ffffffff804505e2:       c9                      leaveq
> ffffffff804505e3:       c3                      retq

And that is due to us lacking kernel commit
e0a96129db574d6365e3439d16d88517c437ab33.

Nevertheless, the existing checks against 0 in the POSIX layer weren't
correct either. We need to fix both, but less ad-hoc than I tried.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Obviously a conversion error while switching to __xn_safe*.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>> Well, I have just checked the kernel code, and 0 as a return value of
>>>>>>>> strncpy_from_user is treated as a value in most places, even if not -EFAULT.
>>>>>>>>
>>>>>>> Better check our code: :) __xn_safe_strncpy_from_user works differently.
>>>>>> Then I would tend to consider that xn_safe_strncpy is broken.
>>>>> No, because it not a derivate of strncpy_from_user, but an internal
>>>>> service optimized for the most common use cases (where you don't care
>>>>> about the precise return value).
>>>> So, what should I call if I care about the return value ?
>>> The old combo of access_rok() and __xn_strncpy_from_user() - ah, I see
>>> the issue: POSIX requires the length to report overflows to the users.
>>> Hmm, then back to the old code, just adding the missing address range check.
>>>
>> Forget that, now I should have looked into our code: The "safe" version
>> transparently calls __xn_strncpy_from_user, no need to go by foot here.
>>
> 
> Here is the real bug:
> 
>>         long res;
>>         __do_strncpy_from_user(dst, src, count, res);
>> ffffffff804505cc:       48 85 c9                test   %rcx,%rcx
>> ffffffff804505cf:       74 0e                   je     ffffffff804505df <rthal_strncpy_from_user+0x1f>
>> ffffffff804505d1:       ac                      lods   %ds:(%rsi),%al
>> ffffffff804505d2:       aa                      stos   %al,%es:(%rdi)
>> ffffffff804505d3:       84 c0                   test   %al,%al
>> ffffffff804505d5:       74 05                   je     ffffffff804505dc <rthal_strncpy_from_user+0x1c>
>> ffffffff804505d7:       48 ff c9                dec    %rcx
>> ffffffff804505da:       75 f5                   jne    ffffffff804505d1 <rthal_strncpy_from_user+0x11>
>> ffffffff804505dc:       48 29 c9                sub    %rcx,%rcx
>                                                          ^^^^^^^^^
> 
>>         return res;
>> }
>> ffffffff804505df:       48 89 c8                mov    %rcx,%rax
>> ffffffff804505e2:       c9                      leaveq
>> ffffffff804505e3:       c3                      retq
> 
> And that is due to us lacking kernel commit
> e0a96129db574d6365e3439d16d88517c437ab33.
> 
> Nevertheless, the existing checks against 0 in the POSIX layer weren't
> correct either. We need to fix both, but less ad-hoc than I tried.

IMO, sem_open(""), mq_open(""), shm_open("") should return an error. I
agree that -EFAULT is a bit harsh and that -EINVAL would be more
apropriate, but I do not want to remove that check.

-- 
					    Gilles.

[Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Jan Kiszka]

Do not return -EFAULT when the passed string has zero-length. Instead,
return -EINVAL when trying to create objects with empty names.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---

 ksrc/skins/posix/syscall.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/ksrc/skins/posix/syscall.c b/ksrc/skins/posix/syscall.c
index 16303b3..c7950a6 100644
--- a/ksrc/skins/posix/syscall.c
+++ b/ksrc/skins/posix/syscall.c
@@ -600,6 +600,8 @@ static int __sem_open(struct pt_regs *regs)
 
 	if (len >= sizeof(name))
 		return -ENAMETOOLONG;
+	if (len == 0)
+		return -EINVAL;
 
 	oflags = __xn_reg_arg3(regs);
 
@@ -1663,11 +1665,13 @@ static int __mq_open(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))
 		return -ENAMETOOLONG;
+	if (len == 0)
+		return -EINVAL;
 
 	oflags = __xn_reg_arg2(regs);
 	mode = __xn_reg_arg3(regs);
@@ -1736,7 +1740,7 @@ static int __mq_unlink(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))
@@ -2440,11 +2444,13 @@ static int __shm_open(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))
 		return -ENAMETOOLONG;
+	if (len == 0)
+		return -EINVAL;
 
 	oflag = (int)__xn_reg_arg2(regs);
 	mode = (mode_t) __xn_reg_arg3(regs);
@@ -2487,7 +2493,7 @@ static int __shm_unlink(struct pt_regs *regs)
 	len = __xn_safe_strncpy_from_user(name,
 					  (const char __user *)__xn_reg_arg1(regs),
 					  sizeof(name));
-	if (len <= 0)
+	if (len < 0)
 		return -EFAULT;
 
 	if (len >= sizeof(name))

Re: [Xenomai-core] [PATCH] posix: Fix error checks when copying user strings

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Do not return -EFAULT when the passed string has zero-length. Instead,
> return -EINVAL when trying to create objects with empty names.

Ok. You can commit this.

-- 
                                                 Gilles.