* [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
@ 2017-10-13 20:39 Dave Hansen
2017-10-13 21:03 ` Andy Lutomirski
0 siblings, 1 reply; 7+ messages in thread
From: Dave Hansen @ 2017-10-13 20:39 UTC (permalink / raw)
To: linux-kernel; +Cc: Dave Hansen, x86, luto
I noticed that we don't have tracepoints for sys_modify_ldt(). I
think that's because we define it directly instead of using the
normal SYSCALL_DEFINEx() macros.
Is there a reason for that, or were they just missed when the
macros were created?
Cc: x86@kernel.org
Cc: Andy Lutomirski <luto@kernel.org>
---
b/arch/x86/include/asm/syscalls.h | 2 +-
b/arch/x86/kernel/ldt.c | 5 +++--
b/arch/x86/um/ldt.c | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
diff -puN arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt arch/x86/kernel/ldt.c
--- a/arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.802553391 -0700
+++ b/arch/x86/kernel/ldt.c 2017-10-13 13:30:12.817553391 -0700
@@ -12,6 +12,7 @@
#include <linux/string.h>
#include <linux/mm.h>
#include <linux/smp.h>
+#include <linux/syscalls.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/uaccess.h>
@@ -294,8 +295,8 @@ out:
return error;
}
-asmlinkage int sys_modify_ldt(int func, void __user *ptr,
- unsigned long bytecount)
+SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
+ unsigned long , bytecount)
{
int ret = -ENOSYS;
diff -puN arch/x86/include/asm/syscalls.h~x86-syscall-macros-modify_ldt arch/x86/include/asm/syscalls.h
--- a/arch/x86/include/asm/syscalls.h~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.812553391 -0700
+++ b/arch/x86/include/asm/syscalls.h 2017-10-13 13:30:12.818553391 -0700
@@ -21,7 +21,7 @@ asmlinkage long sys_ioperm(unsigned long
asmlinkage long sys_iopl(unsigned int);
/* kernel/ldt.c */
-asmlinkage int sys_modify_ldt(int, void __user *, unsigned long);
+asmlinkage long sys_modify_ldt(int, void __user *, unsigned long);
/* kernel/signal.c */
asmlinkage long sys_rt_sigreturn(void);
diff -puN arch/x86/um/ldt.c~x86-syscall-macros-modify_ldt arch/x86/um/ldt.c
--- a/arch/x86/um/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.814553391 -0700
+++ b/arch/x86/um/ldt.c 2017-10-13 13:30:12.818553391 -0700
@@ -369,7 +369,8 @@ void free_ldt(struct mm_context *mm)
mm->arch.ldt.entry_count = 0;
}
-int sys_modify_ldt(int func, void __user *ptr, unsigned long bytecount)
+SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
+ unsigned long , bytecount)
{
return do_modify_ldt_skas(func, ptr, bytecount);
}
_
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
2017-10-13 20:39 [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt() Dave Hansen
@ 2017-10-13 21:03 ` Andy Lutomirski
2017-10-13 22:21 ` Dave Hansen
2017-10-13 23:49 ` Brian Gerst
0 siblings, 2 replies; 7+ messages in thread
From: Andy Lutomirski @ 2017-10-13 21:03 UTC (permalink / raw)
To: Dave Hansen; +Cc: linux-kernel, X86 ML, Andrew Lutomirski
On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> I noticed that we don't have tracepoints for sys_modify_ldt(). I
> think that's because we define it directly instead of using the
> normal SYSCALL_DEFINEx() macros.
>
> Is there a reason for that, or were they just missed when the
> macros were created?
No, and it's a longstanding fsckup that I think you can't fix like
this because...
>
> Cc: x86@kernel.org
> Cc: Andy Lutomirski <luto@kernel.org>
>
> ---
>
> b/arch/x86/include/asm/syscalls.h | 2 +-
> b/arch/x86/kernel/ldt.c | 5 +++--
> b/arch/x86/um/ldt.c | 3 ++-
> 3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff -puN arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt arch/x86/kernel/ldt.c
> --- a/arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.802553391 -0700
> +++ b/arch/x86/kernel/ldt.c 2017-10-13 13:30:12.817553391 -0700
> @@ -12,6 +12,7 @@
> #include <linux/string.h>
> #include <linux/mm.h>
> #include <linux/smp.h>
> +#include <linux/syscalls.h>
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
> #include <linux/uaccess.h>
> @@ -294,8 +295,8 @@ out:
> return error;
> }
>
> -asmlinkage int sys_modify_ldt(int func, void __user *ptr,
> - unsigned long bytecount)
> +SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
> + unsigned long , bytecount)
sys_modify_ldt() returns int, which is wrong, and it's visibly wrong
to 64-bit user code. So I think you need to make sure that the return
value is cast to int in all cases.
--Andy
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
2017-10-13 21:03 ` Andy Lutomirski
@ 2017-10-13 22:21 ` Dave Hansen
2017-10-13 23:49 ` Brian Gerst
1 sibling, 0 replies; 7+ messages in thread
From: Dave Hansen @ 2017-10-13 22:21 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: linux-kernel, X86 ML
On 10/13/2017 02:03 PM, Andy Lutomirski wrote:
>>
>> -asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>> - unsigned long bytecount)
>> +SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
>> + unsigned long , bytecount)
> sys_modify_ldt() returns int, which is wrong, and it's visibly wrong
> to 64-bit user code. So I think you need to make sure that the return
> value is cast to int in all cases.
I'm not quite following.
Is there any difference between having something return 'int' and having
it return 'long' but only use the lower 32 bits? The caller is surely
expecting its result in the lower 32 bits, but this should not change that.
Did you just mean that we need to careful to cast the result in
sys_modify_ldt() over to an 'int' before returning it?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
2017-10-13 21:03 ` Andy Lutomirski
2017-10-13 22:21 ` Dave Hansen
@ 2017-10-13 23:49 ` Brian Gerst
2017-10-14 4:42 ` Andy Lutomirski
1 sibling, 1 reply; 7+ messages in thread
From: Brian Gerst @ 2017-10-13 23:49 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Dave Hansen, linux-kernel, X86 ML
On Fri, Oct 13, 2017 at 5:03 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
>>
>> I noticed that we don't have tracepoints for sys_modify_ldt(). I
>> think that's because we define it directly instead of using the
>> normal SYSCALL_DEFINEx() macros.
>>
>> Is there a reason for that, or were they just missed when the
>> macros were created?
>
> No, and it's a longstanding fsckup that I think you can't fix like
> this because...
>
>>
>> Cc: x86@kernel.org
>> Cc: Andy Lutomirski <luto@kernel.org>
>>
>> ---
>>
>> b/arch/x86/include/asm/syscalls.h | 2 +-
>> b/arch/x86/kernel/ldt.c | 5 +++--
>> b/arch/x86/um/ldt.c | 3 ++-
>> 3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff -puN arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt arch/x86/kernel/ldt.c
>> --- a/arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.802553391 -0700
>> +++ b/arch/x86/kernel/ldt.c 2017-10-13 13:30:12.817553391 -0700
>> @@ -12,6 +12,7 @@
>> #include <linux/string.h>
>> #include <linux/mm.h>
>> #include <linux/smp.h>
>> +#include <linux/syscalls.h>
>> #include <linux/slab.h>
>> #include <linux/vmalloc.h>
>> #include <linux/uaccess.h>
>> @@ -294,8 +295,8 @@ out:
>> return error;
>> }
>>
>> -asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>> - unsigned long bytecount)
>> +SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
>> + unsigned long , bytecount)
>
> sys_modify_ldt() returns int, which is wrong, and it's visibly wrong
> to 64-bit user code. So I think you need to make sure that the return
> value is cast to int in all cases.
I don't think there will be a problem here. If 64-bit userspace
treats it as an int, it will truncate to 32-bit signed and all is
well. If it is treating it as a long, then it's currently broken for
errors anyways.
--
Brian Gerst
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
2017-10-13 23:49 ` Brian Gerst
@ 2017-10-14 4:42 ` Andy Lutomirski
2017-10-14 6:25 ` Brian Gerst
0 siblings, 1 reply; 7+ messages in thread
From: Andy Lutomirski @ 2017-10-14 4:42 UTC (permalink / raw)
To: Brian Gerst; +Cc: Andy Lutomirski, Dave Hansen, linux-kernel, X86 ML
On Fri, Oct 13, 2017 at 4:49 PM, Brian Gerst <brgerst@gmail.com> wrote:
> On Fri, Oct 13, 2017 at 5:03 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen
>> <dave.hansen@linux.intel.com> wrote:
>>>
>>> I noticed that we don't have tracepoints for sys_modify_ldt(). I
>>> think that's because we define it directly instead of using the
>>> normal SYSCALL_DEFINEx() macros.
>>>
>>> Is there a reason for that, or were they just missed when the
>>> macros were created?
>>
>> No, and it's a longstanding fsckup that I think you can't fix like
>> this because...
>>
>>>
>>> Cc: x86@kernel.org
>>> Cc: Andy Lutomirski <luto@kernel.org>
>>>
>>> ---
>>>
>>> b/arch/x86/include/asm/syscalls.h | 2 +-
>>> b/arch/x86/kernel/ldt.c | 5 +++--
>>> b/arch/x86/um/ldt.c | 3 ++-
>>> 3 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff -puN arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt arch/x86/kernel/ldt.c
>>> --- a/arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.802553391 -0700
>>> +++ b/arch/x86/kernel/ldt.c 2017-10-13 13:30:12.817553391 -0700
>>> @@ -12,6 +12,7 @@
>>> #include <linux/string.h>
>>> #include <linux/mm.h>
>>> #include <linux/smp.h>
>>> +#include <linux/syscalls.h>
>>> #include <linux/slab.h>
>>> #include <linux/vmalloc.h>
>>> #include <linux/uaccess.h>
>>> @@ -294,8 +295,8 @@ out:
>>> return error;
>>> }
>>>
>>> -asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>>> - unsigned long bytecount)
>>> +SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
>>> + unsigned long , bytecount)
>>
>> sys_modify_ldt() returns int, which is wrong, and it's visibly wrong
>> to 64-bit user code. So I think you need to make sure that the return
>> value is cast to int in all cases.
>
> I don't think there will be a problem here. If 64-bit userspace
> treats it as an int, it will truncate to 32-bit signed and all is
> well. If it is treating it as a long, then it's currently broken for
> errors anyways.
>
Let me say what I mean more clearly:
The current code is buggy: specifically, a 64-bit modify_ldt() call
that *fails* will return something like (int)-EFAULT. This is bogus,
but it's the ABI. There's even a selftest in the kernel tree that
notices this (although it doesn't check it right now). All that needs
to happen for this patch to be okay AFAIK is to make sure that we
preserve that bug instead of accidentally fixing it.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
2017-10-14 4:42 ` Andy Lutomirski
@ 2017-10-14 6:25 ` Brian Gerst
2017-10-14 16:35 ` Andy Lutomirski
0 siblings, 1 reply; 7+ messages in thread
From: Brian Gerst @ 2017-10-14 6:25 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Dave Hansen, linux-kernel, X86 ML
On Sat, Oct 14, 2017 at 12:42 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Oct 13, 2017 at 4:49 PM, Brian Gerst <brgerst@gmail.com> wrote:
>> On Fri, Oct 13, 2017 at 5:03 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen
>>> <dave.hansen@linux.intel.com> wrote:
>>>>
>>>> I noticed that we don't have tracepoints for sys_modify_ldt(). I
>>>> think that's because we define it directly instead of using the
>>>> normal SYSCALL_DEFINEx() macros.
>>>>
>>>> Is there a reason for that, or were they just missed when the
>>>> macros were created?
>>>
>>> No, and it's a longstanding fsckup that I think you can't fix like
>>> this because...
>>>
>>>>
>>>> Cc: x86@kernel.org
>>>> Cc: Andy Lutomirski <luto@kernel.org>
>>>>
>>>> ---
>>>>
>>>> b/arch/x86/include/asm/syscalls.h | 2 +-
>>>> b/arch/x86/kernel/ldt.c | 5 +++--
>>>> b/arch/x86/um/ldt.c | 3 ++-
>>>> 3 files changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff -puN arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt arch/x86/kernel/ldt.c
>>>> --- a/arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.802553391 -0700
>>>> +++ b/arch/x86/kernel/ldt.c 2017-10-13 13:30:12.817553391 -0700
>>>> @@ -12,6 +12,7 @@
>>>> #include <linux/string.h>
>>>> #include <linux/mm.h>
>>>> #include <linux/smp.h>
>>>> +#include <linux/syscalls.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/vmalloc.h>
>>>> #include <linux/uaccess.h>
>>>> @@ -294,8 +295,8 @@ out:
>>>> return error;
>>>> }
>>>>
>>>> -asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>>>> - unsigned long bytecount)
>>>> +SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
>>>> + unsigned long , bytecount)
>>>
>>> sys_modify_ldt() returns int, which is wrong, and it's visibly wrong
>>> to 64-bit user code. So I think you need to make sure that the return
>>> value is cast to int in all cases.
>>
>> I don't think there will be a problem here. If 64-bit userspace
>> treats it as an int, it will truncate to 32-bit signed and all is
>> well. If it is treating it as a long, then it's currently broken for
>> errors anyways.
>>
>
> Let me say what I mean more clearly:
>
> The current code is buggy: specifically, a 64-bit modify_ldt() call
> that *fails* will return something like (int)-EFAULT. This is bogus,
> but it's the ABI. There's even a selftest in the kernel tree that
> notices this (although it doesn't check it right now). All that needs
> to happen for this patch to be okay AFAIK is to make sure that we
> preserve that bug instead of accidentally fixing it.
return (unsigned int)ret;
Problem solved.
--
Brian Gerst
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()
2017-10-14 6:25 ` Brian Gerst
@ 2017-10-14 16:35 ` Andy Lutomirski
0 siblings, 0 replies; 7+ messages in thread
From: Andy Lutomirski @ 2017-10-14 16:35 UTC (permalink / raw)
To: Brian Gerst; +Cc: Andy Lutomirski, Dave Hansen, linux-kernel, X86 ML
On Fri, Oct 13, 2017 at 11:25 PM, Brian Gerst <brgerst@gmail.com> wrote:
> On Sat, Oct 14, 2017 at 12:42 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Fri, Oct 13, 2017 at 4:49 PM, Brian Gerst <brgerst@gmail.com> wrote:
>>> On Fri, Oct 13, 2017 at 5:03 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>>> On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen
>>>> <dave.hansen@linux.intel.com> wrote:
>>>>>
>>>>> I noticed that we don't have tracepoints for sys_modify_ldt(). I
>>>>> think that's because we define it directly instead of using the
>>>>> normal SYSCALL_DEFINEx() macros.
>>>>>
>>>>> Is there a reason for that, or were they just missed when the
>>>>> macros were created?
>>>>
>>>> No, and it's a longstanding fsckup that I think you can't fix like
>>>> this because...
>>>>
>>>>>
>>>>> Cc: x86@kernel.org
>>>>> Cc: Andy Lutomirski <luto@kernel.org>
>>>>>
>>>>> ---
>>>>>
>>>>> b/arch/x86/include/asm/syscalls.h | 2 +-
>>>>> b/arch/x86/kernel/ldt.c | 5 +++--
>>>>> b/arch/x86/um/ldt.c | 3 ++-
>>>>> 3 files changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff -puN arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt arch/x86/kernel/ldt.c
>>>>> --- a/arch/x86/kernel/ldt.c~x86-syscall-macros-modify_ldt 2017-10-13 13:30:12.802553391 -0700
>>>>> +++ b/arch/x86/kernel/ldt.c 2017-10-13 13:30:12.817553391 -0700
>>>>> @@ -12,6 +12,7 @@
>>>>> #include <linux/string.h>
>>>>> #include <linux/mm.h>
>>>>> #include <linux/smp.h>
>>>>> +#include <linux/syscalls.h>
>>>>> #include <linux/slab.h>
>>>>> #include <linux/vmalloc.h>
>>>>> #include <linux/uaccess.h>
>>>>> @@ -294,8 +295,8 @@ out:
>>>>> return error;
>>>>> }
>>>>>
>>>>> -asmlinkage int sys_modify_ldt(int func, void __user *ptr,
>>>>> - unsigned long bytecount)
>>>>> +SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
>>>>> + unsigned long , bytecount)
>>>>
>>>> sys_modify_ldt() returns int, which is wrong, and it's visibly wrong
>>>> to 64-bit user code. So I think you need to make sure that the return
>>>> value is cast to int in all cases.
>>>
>>> I don't think there will be a problem here. If 64-bit userspace
>>> treats it as an int, it will truncate to 32-bit signed and all is
>>> well. If it is treating it as a long, then it's currently broken for
>>> errors anyways.
>>>
>>
>> Let me say what I mean more clearly:
>>
>> The current code is buggy: specifically, a 64-bit modify_ldt() call
>> that *fails* will return something like (int)-EFAULT. This is bogus,
>> but it's the ABI. There's even a selftest in the kernel tree that
>> notices this (although it doesn't check it right now). All that needs
>> to happen for this patch to be okay AFAIK is to make sure that we
>> preserve that bug instead of accidentally fixing it.
>
> return (unsigned int)ret;
>
> Problem solved.
Agreed. But probably with a comment.
>
> --
> Brian Gerst
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-14 16:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-13 20:39 [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt() Dave Hansen
2017-10-13 21:03 ` Andy Lutomirski
2017-10-13 22:21 ` Dave Hansen
2017-10-13 23:49 ` Brian Gerst
2017-10-14 4:42 ` Andy Lutomirski
2017-10-14 6:25 ` Brian Gerst
2017-10-14 16:35 ` Andy Lutomirski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).