PROBLEM: fanotify_mark EFAULT on x86

PROBLEM: fanotify_mark EFAULT on x86

[Paweł Jasiak]

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

I am trying to run examples from man fanotify.7 but fanotify_mark always
fail with errno = EFAULT.

fanotify_mark declaration is

SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
			      __u64, mask, int, dfd,
			      const char  __user *, pathname)

When 

fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR,
              FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de)

is called on kernel side I can see in do_syscall_32_irqs_on that CPU
context is
    bx = 0x4        = 4
    cx = 0x9        = FAN_MARK_ADD | FAN_MARK_ONLYDIR,
    dx = 0x40000100 = FAN_CREATE | FAN_ONDIR
    si = 0x0
    di = 0xffffff9c = AT_FDCWD
    bp = 0xdeadc0de
    ax = 0xffffffda
    orix_ax = 0x153

I am not sure if it is ok because third argument is uint64_t so if I
understand correctly mask should be divided into two registers (dx and
si).

But in fanotify_mark we get
    fanotify_fd = 4          = bx
    flags       = 0x9        = cx
    mask        = 0x40000100 = dx
    dfd         = 0          = si
    pathname    = 0xffffff9c = di

I believe that correct order is
    fanotify_fd = 4          = bx
    flags       = 0x9        = cx
    mask        = 0x40000100 = (si << 32) | dx
    dfd         = 0xffffff9c = di
    pathname    = 0xdeadc0de = bp

I think that we should call COMPAT version of fanotify_mark here

COMPAT_SYSCALL_DEFINE6(fanotify_mark,
				int, fanotify_fd, unsigned int, flags,
				__u32, mask0, __u32, mask1, int, dfd,
				const char  __user *, pathname)

or something wrong is with 64-bits arguments.

I am running Linux 5.9.2 i686 on Pentium III (Coppermine).
For tests I am using Debian sid on qemu with 5.9.2 and default kernel
from repositories.

Everything works fine on 5.5 and 5.4.

-- 

Paweł Jasiak

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: PROBLEM: fanotify_mark EFAULT on x86

[Matthew Wilcox]

On Sun, Nov 01, 2020 at 10:27:38PM +0100, Paweł Jasiak wrote:
> I am trying to run examples from man fanotify.7 but fanotify_mark always
> fail with errno = EFAULT.
> 
> fanotify_mark declaration is
> 
> SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
> 			      __u64, mask, int, dfd,
> 			      const char  __user *, pathname)

Don't worry about that.  You aren't calling the SYSCALL, you're calling
glibc and glibc is turning it into a syscall.

extern int fanotify_mark (int __fanotify_fd, unsigned int __flags,
                          uint64_t __mask, int __dfd, const char *__pathname)

> When 
> 
> fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR,
>               FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de)

The last argument is supposed to be a pointer to a string.  I'm guessing
there's no string at 0xdeadc0de.

Re: PROBLEM: fanotify_mark EFAULT on x86

[Paweł Jasiak]

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

On 01/11/20, Matthew Wilcox wrote:
> On Sun, Nov 01, 2020 at 10:27:38PM +0100, Paweł Jasiak wrote:
> > I am trying to run examples from man fanotify.7 but fanotify_mark always
> > fail with errno = EFAULT.
> > 
> > fanotify_mark declaration is
> > 
> > SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
> > 			      __u64, mask, int, dfd,
> > 			      const char  __user *, pathname)
> 
> Don't worry about that.  You aren't calling the SYSCALL, you're calling
> glibc and glibc is turning it into a syscall.
> 
> extern int fanotify_mark (int __fanotify_fd, unsigned int __flags,
>                           uint64_t __mask, int __dfd, const char *__pathname)
> 
> > When 
> > 
> > fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR,
> >               FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de)
> 
> The last argument is supposed to be a pointer to a string.  I'm guessing
> there's no string at 0xdeadc0de.

You are right but it's not a problem. 0xdeadc0de is just a _well
known_ address here only for debug purpose.

pathname inside kernel should be a pointer to string located in
user space at 0xdeadc0de but it is equal to 0xffffff9c which is
AT_FDCWD.

If you call

fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_ONLYDIR, FAN_CREATE |
              FAN_ONDIR, AT_FDCWD, argv[1]);

from example with *valid* pointer at argv[1] you still get EFAULT
because pathname is equal to AT_FDCWD in kernel space -- last argument
is not used.

In my example in user space we have
    fanotify_fd = 4
    flags       = 0x9
    mask        = 0x40000100
    dfd         = 0xffffff9c
    pathname    = 0xdeadc0de

and in kernel space we have
    fanotify_fd = 4
    flags       = 0x9
    mask        = 0x40000100
    dfd         = 0
    pathname    = 0xffffff9c

So all arguments after __u64 mask are shifted by one.

It looks similar to https://lists.linux.it/pipermail/ltp/2020-June/017436.html

-- 

Paweł Jasiak

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: PROBLEM: fanotify_mark EFAULT on x86

[Jan Kara]

On Sun 01-11-20 22:27:38, Paweł Jasiak wrote:
> I am trying to run examples from man fanotify.7 but fanotify_mark always
> fail with errno = EFAULT.
> 
> fanotify_mark declaration is
> 
> SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
> 			      __u64, mask, int, dfd,
> 			      const char  __user *, pathname)
> 
> When 
> 
> fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR,
>               FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de)
> 
> is called on kernel side I can see in do_syscall_32_irqs_on that CPU
> context is
>     bx = 0x4        = 4
>     cx = 0x9        = FAN_MARK_ADD | FAN_MARK_ONLYDIR,
>     dx = 0x40000100 = FAN_CREATE | FAN_ONDIR
>     si = 0x0
>     di = 0xffffff9c = AT_FDCWD
>     bp = 0xdeadc0de
>     ax = 0xffffffda
>     orix_ax = 0x153
> 
> I am not sure if it is ok because third argument is uint64_t so if I
> understand correctly mask should be divided into two registers (dx and
> si).
> 
> But in fanotify_mark we get
>     fanotify_fd = 4          = bx
>     flags       = 0x9        = cx
>     mask        = 0x40000100 = dx
>     dfd         = 0          = si
>     pathname    = 0xffffff9c = di
> 
> I believe that correct order is
>     fanotify_fd = 4          = bx
>     flags       = 0x9        = cx
>     mask        = 0x40000100 = (si << 32) | dx
>     dfd         = 0xffffff9c = di
>     pathname    = 0xdeadc0de = bp
> 
> I think that we should call COMPAT version of fanotify_mark here
> 
> COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> 				int, fanotify_fd, unsigned int, flags,
> 				__u32, mask0, __u32, mask1, int, dfd,
> 				const char  __user *, pathname)
> 
> or something wrong is with 64-bits arguments.
> 
> I am running Linux 5.9.2 i686 on Pentium III (Coppermine).
> For tests I am using Debian sid on qemu with 5.9.2 and default kernel
> from repositories.
> 
> Everything works fine on 5.5 and 5.4.

Strange. Thanks for report. Looks like some issue got created / exposed
somewhere between 5.5 and 5.9 (actually probably between 5.5 and 5.7
because the Linaro report you mentioned [1] is from 5.7-rc6). There were
no changes in this area in fanotify, I think it must have been some x86
change that triggered this. Hum, looking into x86 changelog in that time
range there was a series rewriting 32-bit ABI [2] that got merged into
5.7-rc1. Can you perhaps check whether 5.6 is good and 5.7-rc1 is bad?

Brian, any idea whether your series could regress fanotify_mark(2) syscall?
Do we have somewhere documented which syscalls need compat wrappers and how
they should look like?

								Honza

[1] https://lists.linux.it/pipermail/ltp/2020-June/017436.html
[2] https://lore.kernel.org/lkml/20200313195144.164260-1-brgerst@gmail.com/

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

Re: PROBLEM: fanotify_mark EFAULT on x86

[Paweł Jasiak]

On 02/11/20, Jan Kara wrote:
> Strange. Thanks for report. Looks like some issue got created / exposed
> somewhere between 5.5 and 5.9 (actually probably between 5.5 and 5.7
> because the Linaro report you mentioned [1] is from 5.7-rc6). There were
> no changes in this area in fanotify, I think it must have been some x86
> change that triggered this. Hum, looking into x86 changelog in that time
> range there was a series rewriting 32-bit ABI [2] that got merged into
> 5.7-rc1. Can you perhaps check whether 5.6 is good and 5.7-rc1 is bad?

5.6 works.
5.7-rc1 doesn't work.

-- 

Paweł Jasiak

Re: PROBLEM: fanotify_mark EFAULT on x86

[Paweł Jasiak]

I have written small patch that fixes problem for me and doesn't break
x86_64.

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 3e01d8f2ab90..cf0b97309975 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1285,12 +1285,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	return ret;
 }
 
+#if defined(CONFIG_X86) && !defined(CONFIG_64BIT)
+SYSCALL_DEFINE6(fanotify_mark,
+			int, fanotify_fd, unsigned int, flags, __u32, mask0,
+			__u32, mask1, int, dfd, const char  __user *, pathname)
+{
+	return do_fanotify_mark(fanotify_fd, flags,
+#ifdef __BIG_ENDIAN
+				((__u64)mask0 << 32) | mask1,
+#else
+				((__u64)mask1 << 32) | mask0,
+#endif
+				 dfd, pathname);
+}
+#else
 SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 			      __u64, mask, int, dfd,
 			      const char  __user *, pathname)
 {
 	return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname);
 }
+#endif
 
 #ifdef CONFIG_COMPAT
 COMPAT_SYSCALL_DEFINE6(fanotify_mark,


-- 

Paweł Jasiak

Re: PROBLEM: fanotify_mark EFAULT on x86

[Jan Kara]

On Tue 03-11-20 22:17:47, Paweł Jasiak wrote:
> I have written small patch that fixes problem for me and doesn't break
> x86_64.

Yeah, that looks sensible, thanks for the patch. But I'm waiting for some
explanation from x86 folks when compat handlers are really needed and why
it wasn't needed before syscall wrapper rewrite in 5.7-rc1 and is needed
now. Brian, Andy, Thomas?

								Honza

> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 3e01d8f2ab90..cf0b97309975 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1285,12 +1285,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  	return ret;
>  }
>  
> +#if defined(CONFIG_X86) && !defined(CONFIG_64BIT)
> +SYSCALL_DEFINE6(fanotify_mark,
> +			int, fanotify_fd, unsigned int, flags, __u32, mask0,
> +			__u32, mask1, int, dfd, const char  __user *, pathname)
> +{
> +	return do_fanotify_mark(fanotify_fd, flags,
> +#ifdef __BIG_ENDIAN
> +				((__u64)mask0 << 32) | mask1,
> +#else
> +				((__u64)mask1 << 32) | mask0,
> +#endif
> +				 dfd, pathname);
> +}
> +#else
>  SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
>  			      __u64, mask, int, dfd,
>  			      const char  __user *, pathname)
>  {
>  	return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname);
>  }
> +#endif
>  
>  #ifdef CONFIG_COMPAT
>  COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> 
> 
> -- 
> 
> Paweł Jasiak
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: PROBLEM: fanotify_mark EFAULT on x86

[Jan Kara]

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

On Tue 03-11-20 22:17:47, Paweł Jasiak wrote:
> I have written small patch that fixes problem for me and doesn't break
> x86_64.

OK, with a help of Boris Petkov I think I have a fix that looks correct
(attach). Can you please try whether it works for you? Thanks!

								Honza

> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 3e01d8f2ab90..cf0b97309975 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1285,12 +1285,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  	return ret;
>  }
>  
> +#if defined(CONFIG_X86) && !defined(CONFIG_64BIT)
> +SYSCALL_DEFINE6(fanotify_mark,
> +			int, fanotify_fd, unsigned int, flags, __u32, mask0,
> +			__u32, mask1, int, dfd, const char  __user *, pathname)
> +{
> +	return do_fanotify_mark(fanotify_fd, flags,
> +#ifdef __BIG_ENDIAN
> +				((__u64)mask0 << 32) | mask1,
> +#else
> +				((__u64)mask1 << 32) | mask0,
> +#endif
> +				 dfd, pathname);
> +}
> +#else
>  SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
>  			      __u64, mask, int, dfd,
>  			      const char  __user *, pathname)
>  {
>  	return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname);
>  }
> +#endif
>  
>  #ifdef CONFIG_COMPAT
>  COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> 
> 
> -- 
> 
> Paweł Jasiak
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-fanotify-Fix-fanotify_mark-on-32-bit-archs.patch --]
[-- Type: text/x-patch, Size: 2201 bytes --]

From fc9104a50a774ec198c1e3a145372cde77df7967 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 23 Nov 2020 17:37:00 +0100
Subject: [PATCH] fanotify: Fix fanotify_mark() on 32-bit archs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 ABI. Add missed conversion.

CC: Brian Gerst <brgerst@gmail.com>
Suggested-by: Borislav Petkov <bp@suse.de>
Reported-by: Paweł Jasiak <pawel@jasiak.xyz>
Reported-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     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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..e20e7b53a87f 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1293,7 +1293,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 }
 
 #ifdef CONFIG_COMPAT
-COMPAT_SYSCALL_DEFINE6(fanotify_mark,
+SYSCALL_DEFINE6(ia32_fanotify_mark,
 				int, fanotify_fd, unsigned int, flags,
 				__u32, mask0, __u32, mask1, int, dfd,
 				const char  __user *, pathname)
-- 
2.16.4

Re: PROBLEM: fanotify_mark EFAULT on x86

[Paweł Jasiak]

On 23/11/20, Jan Kara wrote:
> OK, with a help of Boris Petkov I think I have a fix that looks correct
> (attach). Can you please try whether it works for you? Thanks!

Unfortunately I am getting a linker error.

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

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

> From fc9104a50a774ec198c1e3a145372cde77df7967 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Mon, 23 Nov 2020 17:37:00 +0100
> Subject: [PATCH] fanotify: Fix fanotify_mark() on 32-bit archs
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> 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 ABI. Add missed conversion.
> 
> CC: Brian Gerst <brgerst@gmail.com>
> Suggested-by: Borislav Petkov <bp@suse.de>
> Reported-by: Paweł Jasiak <pawel@jasiak.xyz>
> Reported-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     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> 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..e20e7b53a87f 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1293,7 +1293,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
>  }
>  
>  #ifdef CONFIG_COMPAT
> -COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> +SYSCALL_DEFINE6(ia32_fanotify_mark,
>  				int, fanotify_fd, unsigned int, flags,
>  				__u32, mask0, __u32, mask1, int, dfd,
>  				const char  __user *, pathname)
> -- 
> 2.16.4
> 


-- 

Paweł Jasiak

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

[kernel test robot]

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

Hi Jan,

I love your patch! Yet something to improve:

[auto build test ERROR on ext3/fsnotify]
[also build test ERROR on linux/master linus/master tip/x86/asm v5.10-rc5 next-20201123]
[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-archs/20201124-004903
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: i386-randconfig-s001-20201123 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-134-gb59dbdaf-dirty
        # https://github.com/0day-ci/linux/commit/db2e8841f458509774f8dcbc5df73be0a91b80d0
        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-archs/20201124-004903
        git checkout db2e8841f458509774f8dcbc5df73be0a91b80d0
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

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+0x54c): 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: 37728 bytes --]

Re: PROBLEM: fanotify_mark EFAULT on x86

[Borislav Petkov]

On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote:
> On 23/11/20, Jan Kara wrote:
> > OK, with a help of Boris Petkov I think I have a fix that looks correct
> > (attach). Can you please try whether it works for you? Thanks!
> 
> Unfortunately I am getting a linker error.
> 
> ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to `__ia32_sys_ia32_fanotify_mark'

Because CONFIG_COMPAT is not set in your .config.

Jan, look at 121b32a58a3a and especially this hunk

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 9b294c13809a..b8f89f78b8cd 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -53,6 +53,8 @@ obj-y                 += setup.o x86_init.o i8259.o irqinit.o
 obj-$(CONFIG_JUMP_LABEL)       += jump_label.o
 obj-$(CONFIG_IRQ_WORK)  += irq_work.o
 obj-y                  += probe_roms.o
+obj-$(CONFIG_X86_32)   += sys_ia32.o
+obj-$(CONFIG_IA32_EMULATION)   += sys_ia32.o

how it enables the ia32 syscalls depending on those config items. Now,
you have

 #ifdef CONFIG_COMPAT
-COMPAT_SYSCALL_DEFINE6(fanotify_mark,
+SYSCALL_DEFINE6(ia32_fanotify_mark,

which is under CONFIG_COMPAT which is not set in Paweł's config.

config COMPAT
        def_bool y
        depends on IA32_EMULATION || X86_X32

but it depends on those two config items.

However, it looks to me like ia32_fanotify_mark's definition should be
simply under CONFIG_X86_32 because:

IA32_EMULATION is 32-bit emulation on 64-bit kernels
X86_X32 is for the x32 ABI

But I could be missing an angle...

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: PROBLEM: fanotify_mark EFAULT on x86

[Jan Kara]

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

On Tue 24-11-20 09:45:07, Borislav Petkov wrote:
> On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote:
> > On 23/11/20, Jan Kara wrote:
> > > OK, with a help of Boris Petkov I think I have a fix that looks correct
> > > (attach). Can you please try whether it works for you? Thanks!
> > 
> > Unfortunately I am getting a linker error.
> > 
> > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to `__ia32_sys_ia32_fanotify_mark'
> 
> Because CONFIG_COMPAT is not set in your .config.
> 
> Jan, look at 121b32a58a3a and especially this hunk
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 9b294c13809a..b8f89f78b8cd 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -53,6 +53,8 @@ obj-y                 += setup.o x86_init.o i8259.o irqinit.o
>  obj-$(CONFIG_JUMP_LABEL)       += jump_label.o
>  obj-$(CONFIG_IRQ_WORK)  += irq_work.o
>  obj-y                  += probe_roms.o
> +obj-$(CONFIG_X86_32)   += sys_ia32.o
> +obj-$(CONFIG_IA32_EMULATION)   += sys_ia32.o
> 
> how it enables the ia32 syscalls depending on those config items. Now,
> you have
> 
>  #ifdef CONFIG_COMPAT
> -COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> +SYSCALL_DEFINE6(ia32_fanotify_mark,
> 
> which is under CONFIG_COMPAT which is not set in Paweł's config.
> 
> config COMPAT
>         def_bool y
>         depends on IA32_EMULATION || X86_X32
> 
> but it depends on those two config items.
> 
> However, it looks to me like ia32_fanotify_mark's definition should be
> simply under CONFIG_X86_32 because:
> 
> IA32_EMULATION is 32-bit emulation on 64-bit kernels
> X86_X32 is for the x32 ABI

Thanks for checking! I didn't realize I needed to change the ifdefs as well
(I missed that bit in 121b32a58a3a). So do I understand correctly that
whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are
passed just fine regardless of whether the userspace is 32-bit or not?

Also how about other 32-bit archs? Because I now realized that
CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by
other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g.
in sparc, powerpc, and other args). So I probably need to actually keep
that for other archs but do the modification only for x86, don't I?

So something like attached patch?

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

[-- Attachment #2: 0001-fanotify-Fix-fanotify_mark-on-32-bit-x86.patch --]
[-- Type: text/x-patch, Size: 2492 bytes --]

From 20d2ddf37c01e91ca18d415a59b3488a394acd8f Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 23 Nov 2020 17:37:00 +0100
Subject: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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-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     | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

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..54a36d4bd116 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1292,8 +1292,12 @@ 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)
+#ifdef CONFIG_X86_32
+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: PROBLEM: fanotify_mark EFAULT on x86

[Borislav Petkov]

On Tue, Nov 24, 2020 at 11:20:33AM +0100, Jan Kara wrote:
> On Tue 24-11-20 09:45:07, Borislav Petkov wrote:
> > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote:
> > > On 23/11/20, Jan Kara wrote:
> > > > OK, with a help of Boris Petkov I think I have a fix that looks correct
> > > > (attach). Can you please try whether it works for you? Thanks!
> > > 
> > > Unfortunately I am getting a linker error.
> > > 
> > > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to `__ia32_sys_ia32_fanotify_mark'
> > 
> > Because CONFIG_COMPAT is not set in your .config.
> > 
> > Jan, look at 121b32a58a3a and especially this hunk
> > 
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index 9b294c13809a..b8f89f78b8cd 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -53,6 +53,8 @@ obj-y                 += setup.o x86_init.o i8259.o irqinit.o
> >  obj-$(CONFIG_JUMP_LABEL)       += jump_label.o
> >  obj-$(CONFIG_IRQ_WORK)  += irq_work.o
> >  obj-y                  += probe_roms.o
> > +obj-$(CONFIG_X86_32)   += sys_ia32.o
> > +obj-$(CONFIG_IA32_EMULATION)   += sys_ia32.o
> > 
> > how it enables the ia32 syscalls depending on those config items. Now,
> > you have
> > 
> >  #ifdef CONFIG_COMPAT
> > -COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> > +SYSCALL_DEFINE6(ia32_fanotify_mark,
> > 
> > which is under CONFIG_COMPAT which is not set in Paweł's config.
> > 
> > config COMPAT
> >         def_bool y
> >         depends on IA32_EMULATION || X86_X32
> > 
> > but it depends on those two config items.
> > 
> > However, it looks to me like ia32_fanotify_mark's definition should be
> > simply under CONFIG_X86_32 because:
> > 
> > IA32_EMULATION is 32-bit emulation on 64-bit kernels
> > X86_X32 is for the x32 ABI
> 
> Thanks for checking! I didn't realize I needed to change the ifdefs as well
> (I missed that bit in 121b32a58a3a). So do I understand correctly that
> whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are
> passed just fine regardless of whether the userspace is 32-bit or not?
> 
> Also how about other 32-bit archs? Because I now realized that
> CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by
> other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g.
> in sparc, powerpc, and other args). So I probably need to actually keep
> that for other archs but do the modification only for x86, don't I?

Hmm, you raise a good point and looking at that commit again, the
intention is to supply those ia32 wrappers as both 32-bit native *and*
32-bit emulation ones.

So I think this

> -#ifdef CONFIG_COMPAT
> +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32)
> +#ifdef CONFIG_X86_32
> +SYSCALL_DEFINE6(ia32_fanotify_mark,
> +#elif CONFIG_COMPAT
>  COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> +#endif

should be

if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
SYSCALL_DEFINE6(ia32_fanotify_mark,
#elif CONFIG_COMPAT
COMPAT_SYSCALL_DEFINE6(fanotify_mark,
#endif

or so.

Meaning that 32-bit native or 32-bit emulation supplies
ia32_fanotify_mark() as a syscall wrapper and other arches doing
CONFIG_COMPAT, still do the COMPAT_SYSCALL_DEFINE6() thing.

But I'd prefer if someone more knowledgeable than me in that whole
syscall macros muck to have a look...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: PROBLEM: fanotify_mark EFAULT on x86

[Naresh Kamboju]

On Tue, 24 Nov 2020 at 15:50, Jan Kara <jack@suse.cz> wrote:
>
> On Tue 24-11-20 09:45:07, Borislav Petkov wrote:
> > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote:
> > > On 23/11/20, Jan Kara wrote:
> > > > OK, with a help of Boris Petkov I think I have a fix that looks correct
> > > > (attach). Can you please try whether it works for you? Thanks!
> > >
<trim>
>
> Thanks for checking! I didn't realize I needed to change the ifdefs as well
> (I missed that bit in 121b32a58a3a). So do I understand correctly that
> whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are
> passed just fine regardless of whether the userspace is 32-bit or not?
>
> Also how about other 32-bit archs? Because I now realized that
> CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by
> other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g.
> in sparc, powerpc, and other args). So I probably need to actually keep
> that for other archs but do the modification only for x86, don't I?
>
> So something like attached patch?

I have tested the attached patch on i386 and qemu_i386 and the reported problem
got fixed.

Test links,
https://lkft.validation.linaro.org/scheduler/job/1985236#L1176
https://lkft.validation.linaro.org/scheduler/job/1985238#L801


-- 
Linaro LKFT
https://lkft.linaro.org

Re: PROBLEM: fanotify_mark EFAULT on x86

[Jan Kara]

On Tue 24-11-20 11:28:14, Borislav Petkov wrote:
> On Tue, Nov 24, 2020 at 11:20:33AM +0100, Jan Kara wrote:
> > On Tue 24-11-20 09:45:07, Borislav Petkov wrote:
> > > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote:
> > > > On 23/11/20, Jan Kara wrote:
> > > > > OK, with a help of Boris Petkov I think I have a fix that looks correct
> > > > > (attach). Can you please try whether it works for you? Thanks!
> > > > 
> > > > Unfortunately I am getting a linker error.
> > > > 
> > > > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to `__ia32_sys_ia32_fanotify_mark'
> > > 
> > > Because CONFIG_COMPAT is not set in your .config.
> > > 
> > > Jan, look at 121b32a58a3a and especially this hunk
> > > 
> > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > > index 9b294c13809a..b8f89f78b8cd 100644
> > > --- a/arch/x86/kernel/Makefile
> > > +++ b/arch/x86/kernel/Makefile
> > > @@ -53,6 +53,8 @@ obj-y                 += setup.o x86_init.o i8259.o irqinit.o
> > >  obj-$(CONFIG_JUMP_LABEL)       += jump_label.o
> > >  obj-$(CONFIG_IRQ_WORK)  += irq_work.o
> > >  obj-y                  += probe_roms.o
> > > +obj-$(CONFIG_X86_32)   += sys_ia32.o
> > > +obj-$(CONFIG_IA32_EMULATION)   += sys_ia32.o
> > > 
> > > how it enables the ia32 syscalls depending on those config items. Now,
> > > you have
> > > 
> > >  #ifdef CONFIG_COMPAT
> > > -COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> > > +SYSCALL_DEFINE6(ia32_fanotify_mark,
> > > 
> > > which is under CONFIG_COMPAT which is not set in Paweł's config.
> > > 
> > > config COMPAT
> > >         def_bool y
> > >         depends on IA32_EMULATION || X86_X32
> > > 
> > > but it depends on those two config items.
> > > 
> > > However, it looks to me like ia32_fanotify_mark's definition should be
> > > simply under CONFIG_X86_32 because:
> > > 
> > > IA32_EMULATION is 32-bit emulation on 64-bit kernels
> > > X86_X32 is for the x32 ABI
> > 
> > Thanks for checking! I didn't realize I needed to change the ifdefs as well
> > (I missed that bit in 121b32a58a3a). So do I understand correctly that
> > whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are
> > passed just fine regardless of whether the userspace is 32-bit or not?
> > 
> > Also how about other 32-bit archs? Because I now realized that
> > CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by
> > other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g.
> > in sparc, powerpc, and other args). So I probably need to actually keep
> > that for other archs but do the modification only for x86, don't I?
> 
> Hmm, you raise a good point and looking at that commit again, the
> intention is to supply those ia32 wrappers as both 32-bit native *and*
> 32-bit emulation ones.
> 
> So I think this
> 
> > -#ifdef CONFIG_COMPAT
> > +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32)
> > +#ifdef CONFIG_X86_32
> > +SYSCALL_DEFINE6(ia32_fanotify_mark,
> > +#elif CONFIG_COMPAT
> >  COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> > +#endif
> 
> should be
> 
> if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
> SYSCALL_DEFINE6(ia32_fanotify_mark,
> #elif CONFIG_COMPAT
> COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> #endif
> 
> or so.
> 
> Meaning that 32-bit native or 32-bit emulation supplies
> ia32_fanotify_mark() as a syscall wrapper and other arches doing
> CONFIG_COMPAT, still do the COMPAT_SYSCALL_DEFINE6() thing.

Yeah, looking again at what those config options mean I agree. Patch
updated.

> But I'd prefer if someone more knowledgeable than me in that whole
> syscall macros muck to have a look...

I'd prefer that as well but if nobody pops up, I'll just push this to my
tree next week and will see what breaks :)

								Honza

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

Re: PROBLEM: fanotify_mark EFAULT on x86

[Jan Kara]

On Thu 26-11-20 01:01:30, Naresh Kamboju wrote:
> On Tue, 24 Nov 2020 at 15:50, Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 24-11-20 09:45:07, Borislav Petkov wrote:
> > > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote:
> > > > On 23/11/20, Jan Kara wrote:
> > > > > OK, with a help of Boris Petkov I think I have a fix that looks correct
> > > > > (attach). Can you please try whether it works for you? Thanks!
> > > >
> <trim>
> >
> > Thanks for checking! I didn't realize I needed to change the ifdefs as well
> > (I missed that bit in 121b32a58a3a). So do I understand correctly that
> > whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are
> > passed just fine regardless of whether the userspace is 32-bit or not?
> >
> > Also how about other 32-bit archs? Because I now realized that
> > CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by
> > other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g.
> > in sparc, powerpc, and other args). So I probably need to actually keep
> > that for other archs but do the modification only for x86, don't I?
> >
> > So something like attached patch?
> 
> I have tested the attached patch on i386 and qemu_i386 and the reported problem
> got fixed.
> 
> Test links,
> https://lkft.validation.linaro.org/scheduler/job/1985236#L1176
> https://lkft.validation.linaro.org/scheduler/job/1985238#L801

Thanks for testing! I've added your tested-by tag.

								Honza

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

Re: PROBLEM: fanotify_mark EFAULT on x86

[Borislav Petkov]

On Thu, Nov 26, 2020 at 11:48:27AM +0100, Jan Kara wrote:
> I'd prefer that as well but if nobody pops up, I'll just push this to my
> tree next week and will see what breaks :)

Right. You could send a proper patch and Cc the usual suspects as now it
is buried in some thread which people might not read.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette