On Fri, Oct 13, 2017 at 11:25 PM, Brian Gerst <brge...@gmail.com> wrote: > 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.
Agreed. But probably with a comment. > > -- > Brian Gerst
