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@xxxxxxxxx> wrote:
> On Fri, Oct 13, 2017 at 5:03 PM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>> On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen
>> <dave.hansen@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx
>>> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
>>>
>>> ---
>>>
>>>  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.