Re: [PATCH v2 1/8] x86-64: Improve vsyscall emulation CS and RIP handling

[Andrew Lutomirski]

On Mon, Jul 11, 2011 at 6:14 PM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Sat, Jul 09, 2011 at 11:22:08PM -0400, Andy Lutomirski wrote:
>> Two fixes here:
>>  - Send SIGSEGV if called from compat code or with a funny CS.
>>  - Don't BUG on impossible addresses.
>>
>> This patch also removes an unused variable.
>>
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxx>
>> ---
>>  arch/x86/include/asm/vsyscall.h |   12 --------
>>  arch/x86/kernel/vsyscall_64.c   |   59 +++++++++++++++++++++++++--------------
>>  2 files changed, 38 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/vsyscall.h b/arch/x86/include/asm/vsyscall.h
>> index bb710cb..d555973 100644
>> --- a/arch/x86/include/asm/vsyscall.h
>> +++ b/arch/x86/include/asm/vsyscall.h
>> @@ -31,18 +31,6 @@ extern struct timezone sys_tz;
>>
>>  extern void map_vsyscall(void);
>>
>> -/* Emulation */
>> -
>> -static inline bool is_vsyscall_entry(unsigned long addr)
>> -{
>> -     return (addr & ~0xC00UL) == VSYSCALL_START;
>> -}
>> -
>> -static inline int vsyscall_entry_nr(unsigned long addr)
>> -{
>> -     return (addr & 0xC00UL) >> 10;
>> -}
>> -
>>  #endif /* __KERNEL__ */
>>
>>  #endif /* _ASM_X86_VSYSCALL_H */
>> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
>> index 10cd8ac..6d14848 100644
>> --- a/arch/x86/kernel/vsyscall_64.c
>> +++ b/arch/x86/kernel/vsyscall_64.c
>> @@ -38,6 +38,7 @@
>>
>>  #include <asm/vsyscall.h>
>>  #include <asm/pgtable.h>
>> +#include <asm/compat.h>
>>  #include <asm/page.h>
>>  #include <asm/unistd.h>
>>  #include <asm/fixmap.h>
>> @@ -97,33 +98,60 @@ static void warn_bad_vsyscall(const char *level, struct pt_regs *regs,
>>
>>       tsk = current;
>>
>> -     printk("%s%s[%d] %s ip:%lx sp:%lx ax:%lx si:%lx di:%lx\n",
>> +     printk("%s%s[%d] %s ip:%lx cs:%lx sp:%lx ax:%lx si:%lx di:%lx\n",
>>              level, tsk->comm, task_pid_nr(tsk),
>> -            message, regs->ip - 2, regs->sp, regs->ax, regs->si, regs->di);
>> +            message, regs->ip - 2, regs->cs,
>> +            regs->sp, regs->ax, regs->si, regs->di);
>> +}
>> +
>> +static int addr_to_vsyscall_nr(unsigned *vsyscall_nr, unsigned long addr)
>> +{
>> +     if ((addr & ~0xC00UL) != VSYSCALL_START)
>> +             return -EINVAL;
>> +
>> +     *vsyscall_nr = (addr & 0xC00UL) >> 10;
>> +     if (*vsyscall_nr >= 3)
>> +             return -EINVAL;
>> +
>> +     return 0;
>>  }
>
> I'm wondering: why don't you make this function return negative value on
> error, i.e. -EINVAL and the vsyscall number on success so that you can
> get rid of returning it through the arg pointer?
>
> Then at the callsite you can do:
>
>        vsyscall_nr = addr_to_vsyscall_nr(addr);
>        if (vsyscall_nr < 0)
>                warn_bad_vsyscall(...)

Because I don't want a warning about ret being used without being initialized.

With the code in this patch, the compiler is smart enough to figure
out that either vsyscall_nr is 0, 1, or 2 or that the EINVAL branch is
taken.  I'll see if it works the other way.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/