On Sat, Oct 14, 2017 at 12:42 AM, Andy Lutomirski <l...@kernel.org> wrote: > On Fri, Oct 13, 2017 at 4:49 PM, Brian Gerst <brge...@gmail.com> wrote: >> On Fri, Oct 13, 2017 at 5:03 PM, Andy Lutomirski <l...@kernel.org> wrote: >>> On Fri, Oct 13, 2017 at 1:39 PM, Dave Hansen >>> <dave.han...@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: x...@kernel.org >>>> Cc: Andy Lutomirski <l...@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
