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

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

[Jan Kara]

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.

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

[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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Hi Jan,

I love your patch! Yet something to improve:

[auto build test ERROR on ext3/fsnotify]
[also build test ERROR on tip/x86/asm v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jan-Kara/fanotify-Fix-fanotify_mark-on-32-bit-x86/20201126-235659
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: x86_64-kexec (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/83512221cb769f80e071541619033b0dad1b8fa6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jan-Kara/fanotify-Fix-fanotify_mark-on-32-bit-x86/20201126-235659
        git checkout 83512221cb769f80e071541619033b0dad1b8fa6
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld: arch/x86/entry/syscall_32.o:(.rodata+0xa98): undefined reference to `__ia32_sys_ia32_fanotify_mark'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28188 bytes --]