[RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Dave Hansen]

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);
 }
_

Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

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

Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Dave Hansen]

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?

Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Brian Gerst]

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

Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Andy Lutomirski]

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.

Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Brian Gerst]

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

Re: [RFC][PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Andy Lutomirski]

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