Re: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86

Re: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86

[Andy Lutomirski]

On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote:
>
> Commit converting syscalls taking 64-bit arguments to new scheme of compat
> handlers omitted converting fanotify_mark(2) which then broke the
> syscall for 32-bit x86 builds. Add missed conversion. It is somewhat
> cumbersome since we need to keep the original compat handler for all the
> other 32-bit archs.
>

This is stupendously ugly.  I'm not really sure how this is supposed
to work on any 32-bit arch.  I'm also not sure whether we should
expect the SYSCALL_DEFINE macros to figure this out by themselves.

At the very least, the native arm 32 and arm64 compat cases should get tested.

Al and Christoph, you're probably a lot more familiar than I am with
the nasty details of syscall ABI with 64-bit arguments.

> CC: Brian Gerst <brgerst@gmail.com>
> Suggested-by: Borislav Petkov <bp@suse.de>
> Reported-by: Paweł Jasiak <pawel@jasiak.xyz>
> Reported-and-tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Fixes: 121b32a58a3a ("x86/entry/32: Use IA32-specific wrappers for syscalls taking 64-bit arguments")
> CC: stable@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  arch/x86/entry/syscalls/syscall_32.tbl | 2 +-
>  fs/notify/fanotify/fanotify_user.c     | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> I plan to queue this fix into my tree next week. I'd be happy if someone with
> x86 ABI knowledge checks whether I've got the patch right (especially various
> config variants) because it was mostly a guesswork of me & Boris ;). Thanks!
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 0d0667a9fbd7..b2ec6ff88307 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -350,7 +350,7 @@
>  336    i386    perf_event_open         sys_perf_event_open
>  337    i386    recvmmsg                sys_recvmmsg_time32             compat_sys_recvmmsg_time32
>  338    i386    fanotify_init           sys_fanotify_init
> -339    i386    fanotify_mark           sys_fanotify_mark               compat_sys_fanotify_mark
> +339    i386    fanotify_mark           sys_ia32_fanotify_mark
>  340    i386    prlimit64               sys_prlimit64
>  341    i386    name_to_handle_at       sys_name_to_handle_at
>  342    i386    open_by_handle_at       sys_open_by_handle_at           compat_sys_open_by_handle_at
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 3e01d8f2ab90..ba38f0fec4d0 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1292,8 +1292,13 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
>         return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname);
>  }
>
> -#ifdef CONFIG_COMPAT
> +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32) || \
> +    defined(CONFIG_IA32_EMULATION)
> +#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
> +SYSCALL_DEFINE6(ia32_fanotify_mark,
> +#elif CONFIG_COMPAT
>  COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> +#endif
>                                 int, fanotify_fd, unsigned int, flags,
>                                 __u32, mask0, __u32, mask1, int, dfd,
>                                 const char  __user *, pathname)
> --
> 2.16.4
>

Re: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86

[Brian Gerst]

On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote:
> >
> > Commit converting syscalls taking 64-bit arguments to new scheme of compat
> > handlers omitted converting fanotify_mark(2) which then broke the
> > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat
> > cumbersome since we need to keep the original compat handler for all the
> > other 32-bit archs.
> >
>
> This is stupendously ugly.  I'm not really sure how this is supposed
> to work on any 32-bit arch.  I'm also not sure whether we should
> expect the SYSCALL_DEFINE macros to figure this out by themselves.

It works on 32-bit arches because the compiler implicitly uses
consecutive input registers or stack slots for 64-bit arguments, and
some arches have alignment requirements that result in hidden padding.
x86-32 is different now because parameters are passed in via pt_regs,
and the 64-bit value has to explicitly be reassembled from the high
and low 32-bit values, just like in the compat case.

I think the simplest way to handle this is add a wrapper in
arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit
args.  That keeps this mess out of general code.

--
Brian Gerst

Re: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86

[Andy Lutomirski]

On Fri, Nov 27, 2020 at 2:30 PM Brian Gerst <brgerst@gmail.com> wrote:
>
> On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Commit converting syscalls taking 64-bit arguments to new scheme of compat
> > > handlers omitted converting fanotify_mark(2) which then broke the
> > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat
> > > cumbersome since we need to keep the original compat handler for all the
> > > other 32-bit archs.
> > >
> >
> > This is stupendously ugly.  I'm not really sure how this is supposed
> > to work on any 32-bit arch.  I'm also not sure whether we should
> > expect the SYSCALL_DEFINE macros to figure this out by themselves.
>
> It works on 32-bit arches because the compiler implicitly uses
> consecutive input registers or stack slots for 64-bit arguments, and
> some arches have alignment requirements that result in hidden padding.
> x86-32 is different now because parameters are passed in via pt_regs,
> and the 64-bit value has to explicitly be reassembled from the high
> and low 32-bit values, just like in the compat case.
>

That was my guess.

> I think the simplest way to handle this is add a wrapper in
> arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit
> args.  That keeps this mess out of general code.


Want to send a patch?

I also wonder if there's a straightforward way to statically check
this.  Maybe the syscall wrapper macros could be rigged up to avoid
emitting the ia32 stubs if there is a u64 or s64 arg, so the build
would fail if someone tries to stick one in the syscall tables.  I
tried to do this, but I got a bit lost in the macro maze and my
attempt didn't work.

--Andy

Re: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86

[Brian Gerst]

On Fri, Nov 27, 2020 at 7:36 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Fri, Nov 27, 2020 at 2:30 PM Brian Gerst <brgerst@gmail.com> wrote:
> >
> > On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote:
> > >
> > > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > Commit converting syscalls taking 64-bit arguments to new scheme of compat
> > > > handlers omitted converting fanotify_mark(2) which then broke the
> > > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat
> > > > cumbersome since we need to keep the original compat handler for all the
> > > > other 32-bit archs.
> > > >
> > >
> > > This is stupendously ugly.  I'm not really sure how this is supposed
> > > to work on any 32-bit arch.  I'm also not sure whether we should
> > > expect the SYSCALL_DEFINE macros to figure this out by themselves.
> >
> > It works on 32-bit arches because the compiler implicitly uses
> > consecutive input registers or stack slots for 64-bit arguments, and
> > some arches have alignment requirements that result in hidden padding.
> > x86-32 is different now because parameters are passed in via pt_regs,
> > and the 64-bit value has to explicitly be reassembled from the high
> > and low 32-bit values, just like in the compat case.
> >
>
> That was my guess.
>
> > I think the simplest way to handle this is add a wrapper in
> > arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit
> > args.  That keeps this mess out of general code.
>
>
> Want to send a patch?

I settled on doing something along the same line as Jan, but in a more
generic way that lays the groundwork for converting more of these
arch-specific compat wrappers to a generic wrapper.

Patch coming soon.

--
Brian Gerst

Re: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86

[Jan Kara]

On Mon 30-11-20 17:21:08, Brian Gerst wrote:
> On Fri, Nov 27, 2020 at 7:36 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On Fri, Nov 27, 2020 at 2:30 PM Brian Gerst <brgerst@gmail.com> wrote:
> > >
> > > On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote:
> > > >
> > > > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > Commit converting syscalls taking 64-bit arguments to new scheme of compat
> > > > > handlers omitted converting fanotify_mark(2) which then broke the
> > > > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat
> > > > > cumbersome since we need to keep the original compat handler for all the
> > > > > other 32-bit archs.
> > > > >
> > > >
> > > > This is stupendously ugly.  I'm not really sure how this is supposed
> > > > to work on any 32-bit arch.  I'm also not sure whether we should
> > > > expect the SYSCALL_DEFINE macros to figure this out by themselves.
> > >
> > > It works on 32-bit arches because the compiler implicitly uses
> > > consecutive input registers or stack slots for 64-bit arguments, and
> > > some arches have alignment requirements that result in hidden padding.
> > > x86-32 is different now because parameters are passed in via pt_regs,
> > > and the 64-bit value has to explicitly be reassembled from the high
> > > and low 32-bit values, just like in the compat case.
> > >
> >
> > That was my guess.
> >
> > > I think the simplest way to handle this is add a wrapper in
> > > arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit
> > > args.  That keeps this mess out of general code.
> >
> >
> > Want to send a patch?
> 
> I settled on doing something along the same line as Jan, but in a more
> generic way that lays the groundwork for converting more of these
> arch-specific compat wrappers to a generic wrapper.

Cool, thanks for looking into this!

> Patch coming soon.

Looking forward to it :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR