Re: [PATCH] x32: Mask away the x32 syscall bit in the ptrace codepath

[Philipp Kern]

On Wed, May 28, 2014 at 10:47 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On 05/28/2014 05:19 AM, Philipp Kern wrote:
> > audit_filter_syscall uses the syscall number to reference into a
> > bitmask (e->rule.mask[word]). Not removing the x32 bit before passing
> > the number to this architecture independent codepath will fail to
> > lookup the proper audit bit. Furthermore it will cause an invalid memory
> > access in the kernel if the out of bound location is not mapped:
> >
> >   BUG: unable to handle kernel paging request at ffff8800e5446630
> >   IP: [<ffffffff810fcdd0>] audit_filter_syscall+0x90/0xf0
> >
> > Together with the entrypoint in entry_64.S this change causes x32
> > programs to pass in both AUDIT_ARCH_X86_64 and AUDIT_ARCH_I386 depending
> > on the syscall path.
> >
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: H. J. Lu <hjl.tools@xxxxxxxxx>
> > Cc: Eric Paris <eparis@xxxxxxxxxx>
> > Signed-off-by: Philipp Kern <pkern@xxxxxxxxxx>
> > ---
> >  arch/x86/kernel/ptrace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > index 678c0ad..166a3c7 100644
> > --- a/arch/x86/kernel/ptrace.c
> > +++ b/arch/x86/kernel/ptrace.c
> > @@ -1489,7 +1489,7 @@ long syscall_trace_enter(struct pt_regs *regs)
> >
> >       if (IS_IA32)
> >               audit_syscall_entry(AUDIT_ARCH_I386,
> > -                                 regs->orig_ax,
> > +                                 regs->orig_ax & __SYSCALL_MASK,
>
> This is weird.  Three questions:
>
> 1. How can this happen?  I thought that x32 syscalls always came in
> through the syscall path, which doesn't set is_compat_task.  (Can
> someone rename is_compat_task to in_compat_syscall?  Pretty please?)

The other patch is this one: https://lkml.org/lkml/2014/5/26/209

But I agree: IS_IA32 is confusing but it does trigger on x32 tasks. I
put that into a bug report but apparently not into the patch
description, I'm sorry. is_compat_task is defined as is_ia32_task() ||
is_x32_task().

> 2. Now audit can't tell whether a syscall is x32 or i386.  And audit is
> inconsistent with seccomp.  This seems wrong.

I'm not sure where seccomp is hooked in. Can you point me to the entry
points here? ptrace still requires that userspace sees the x32 bit so
it cannot be masked beforehand. The path the other patch fixes masks
the bit away directly after the audit call.

> 3. The OOPS you're fixing doesn't seem like it's fixed.  What if some
> other random high bits are set?

Fair point. I guess we should check against AUDIT_BITMASK_SIZE as that
is the size of the array we are referencing into. But what would we do
in case it's larger? Those are not ENOSYS'ed yet in the entry_64.S
path. That's after tracing and auditing.

Kind regards and thanks
Philipp Kern
--
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/