linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
@ 2019-08-21  3:38 Guillem Jover
  2019-08-21 23:55 ` Christoph Hellwig
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Guillem Jover @ 2019-08-21  3:38 UTC (permalink / raw)
  To: linux-aio
  Cc: Christoph Hellwig, Jeff Moyer, Benjamin LaHaise, Alexander Viro,
	linux-fsdevel, linux-kernel

This type is used to pass the sigset_t from userland to the kernel,
but it was using the kernel native pointer type for the member
representing the compat userland pointer to the userland sigset_t.

This messes up the layout, and makes the kernel eat up both the
userland pointer and the size members into the kernel pointer, and
then reads garbage into the kernel sigsetsize. Which makes the sigset_t
size consistency check fail, and consequently the syscall always
returns -EINVAL.

This breaks both libaio and strace on 32-bit userland running on 64-bit
kernels. And there are apparently no users in the wild of the current
broken layout (at least according to codesearch.debian.org and a brief
check over github.com search). So it looks safe to fix this directly
in the kernel, instead of either letting userland deal with this
permanently with the additional overhead or trying to make the syscall
infer what layout userland used, even though this is also being worked
around in libaio to temporarily cope with kernels that have not yet
been fixed.

We use a proper compat_uptr_t instead of a compat_sigset_t pointer.

Fixes: 7a074e96 ("aio: implement io_pgetevents")
Signed-off-by: Guillem Jover <guillem@hadrons.org>
---
 fs/aio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 01e0fb9ae45a..056f291bc66f 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2179,7 +2179,7 @@ SYSCALL_DEFINE5(io_getevents_time32, __u32, ctx_id,
 #ifdef CONFIG_COMPAT
 
 struct __compat_aio_sigset {
-	compat_sigset_t __user	*sigmask;
+	compat_uptr_t		sigmask;
 	compat_size_t		sigsetsize;
 };
 
@@ -2204,7 +2204,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
 		return -EFAULT;
 
-	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
+	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
 	if (ret)
 		return ret;
 
@@ -2239,7 +2239,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
 	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
 		return -EFAULT;
 
-	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
+	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
 	if (ret)
 		return ret;
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
  2019-08-21  3:38 [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout Guillem Jover
@ 2019-08-21 23:55 ` Christoph Hellwig
  2019-08-22 17:53 ` Jeff Moyer
  2019-10-17 13:48 ` Jan Kara
  2 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2019-08-21 23:55 UTC (permalink / raw)
  To: Guillem Jover
  Cc: linux-aio, Christoph Hellwig, Jeff Moyer, Benjamin LaHaise,
	Alexander Viro, linux-fsdevel, linux-kernel

On Wed, Aug 21, 2019 at 05:38:20AM +0200, Guillem Jover wrote:
> This type is used to pass the sigset_t from userland to the kernel,
> but it was using the kernel native pointer type for the member
> representing the compat userland pointer to the userland sigset_t.
> 
> This messes up the layout, and makes the kernel eat up both the
> userland pointer and the size members into the kernel pointer, and
> then reads garbage into the kernel sigsetsize. Which makes the sigset_t
> size consistency check fail, and consequently the syscall always
> returns -EINVAL.
> 
> This breaks both libaio and strace on 32-bit userland running on 64-bit
> kernels. And there are apparently no users in the wild of the current
> broken layout (at least according to codesearch.debian.org and a brief
> check over github.com search). So it looks safe to fix this directly
> in the kernel, instead of either letting userland deal with this
> permanently with the additional overhead or trying to make the syscall
> infer what layout userland used, even though this is also being worked
> around in libaio to temporarily cope with kernels that have not yet
> been fixed.
> 
> We use a proper compat_uptr_t instead of a compat_sigset_t pointer.
> 
> Fixes: 7a074e96 ("aio: implement io_pgetevents")
> Signed-off-by: Guillem Jover <guillem@hadrons.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
  2019-08-21  3:38 [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout Guillem Jover
  2019-08-21 23:55 ` Christoph Hellwig
@ 2019-08-22 17:53 ` Jeff Moyer
  2019-10-17 13:48 ` Jan Kara
  2 siblings, 0 replies; 7+ messages in thread
From: Jeff Moyer @ 2019-08-22 17:53 UTC (permalink / raw)
  To: Guillem Jover
  Cc: linux-aio, Christoph Hellwig, Benjamin LaHaise, Alexander Viro,
	linux-fsdevel, linux-kernel

Guillem Jover <guillem@hadrons.org> writes:

> This type is used to pass the sigset_t from userland to the kernel,
> but it was using the kernel native pointer type for the member
> representing the compat userland pointer to the userland sigset_t.
>
> This messes up the layout, and makes the kernel eat up both the
> userland pointer and the size members into the kernel pointer, and
> then reads garbage into the kernel sigsetsize. Which makes the sigset_t
> size consistency check fail, and consequently the syscall always
> returns -EINVAL.
>
> This breaks both libaio and strace on 32-bit userland running on 64-bit
> kernels. And there are apparently no users in the wild of the current
> broken layout (at least according to codesearch.debian.org and a brief
> check over github.com search). So it looks safe to fix this directly
> in the kernel, instead of either letting userland deal with this
> permanently with the additional overhead or trying to make the syscall
> infer what layout userland used, even though this is also being worked
> around in libaio to temporarily cope with kernels that have not yet
> been fixed.
>
> We use a proper compat_uptr_t instead of a compat_sigset_t pointer.
>
> Fixes: 7a074e96 ("aio: implement io_pgetevents")
> Signed-off-by: Guillem Jover <guillem@hadrons.org>

Looks good, thanks for finding and fixing this!

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  fs/aio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 01e0fb9ae45a..056f291bc66f 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -2179,7 +2179,7 @@ SYSCALL_DEFINE5(io_getevents_time32, __u32, ctx_id,
>  #ifdef CONFIG_COMPAT
>  
>  struct __compat_aio_sigset {
> -	compat_sigset_t __user	*sigmask;
> +	compat_uptr_t		sigmask;
>  	compat_size_t		sigsetsize;
>  };
>  
> @@ -2204,7 +2204,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
>  	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
>  		return -EFAULT;
>  
> -	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
> +	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
>  	if (ret)
>  		return ret;
>  
> @@ -2239,7 +2239,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
>  	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
>  		return -EFAULT;
>  
> -	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
> +	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
>  	if (ret)
>  		return ret;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
  2019-08-21  3:38 [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout Guillem Jover
  2019-08-21 23:55 ` Christoph Hellwig
  2019-08-22 17:53 ` Jeff Moyer
@ 2019-10-17 13:48 ` Jan Kara
  2019-10-21 20:15   ` Al Viro
  2 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2019-10-17 13:48 UTC (permalink / raw)
  To: Guillem Jover
  Cc: linux-aio, Christoph Hellwig, Jeff Moyer, Benjamin LaHaise,
	Alexander Viro, linux-fsdevel, linux-kernel

On Wed 21-08-19 05:38:20, Guillem Jover wrote:
> This type is used to pass the sigset_t from userland to the kernel,
> but it was using the kernel native pointer type for the member
> representing the compat userland pointer to the userland sigset_t.
> 
> This messes up the layout, and makes the kernel eat up both the
> userland pointer and the size members into the kernel pointer, and
> then reads garbage into the kernel sigsetsize. Which makes the sigset_t
> size consistency check fail, and consequently the syscall always
> returns -EINVAL.
> 
> This breaks both libaio and strace on 32-bit userland running on 64-bit
> kernels. And there are apparently no users in the wild of the current
> broken layout (at least according to codesearch.debian.org and a brief
> check over github.com search). So it looks safe to fix this directly
> in the kernel, instead of either letting userland deal with this
> permanently with the additional overhead or trying to make the syscall
> infer what layout userland used, even though this is also being worked
> around in libaio to temporarily cope with kernels that have not yet
> been fixed.
> 
> We use a proper compat_uptr_t instead of a compat_sigset_t pointer.
> 
> Fixes: 7a074e96 ("aio: implement io_pgetevents")
> Signed-off-by: Guillem Jover <guillem@hadrons.org>

This patch seems to have fallen through the cracks. Al?

								Honza

> ---
>  fs/aio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 01e0fb9ae45a..056f291bc66f 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -2179,7 +2179,7 @@ SYSCALL_DEFINE5(io_getevents_time32, __u32, ctx_id,
>  #ifdef CONFIG_COMPAT
>  
>  struct __compat_aio_sigset {
> -	compat_sigset_t __user	*sigmask;
> +	compat_uptr_t		sigmask;
>  	compat_size_t		sigsetsize;
>  };
>  
> @@ -2204,7 +2204,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
>  	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
>  		return -EFAULT;
>  
> -	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
> +	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
>  	if (ret)
>  		return ret;
>  
> @@ -2239,7 +2239,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
>  	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
>  		return -EFAULT;
>  
> -	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
> +	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.23.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
  2019-10-17 13:48 ` Jan Kara
@ 2019-10-21 20:15   ` Al Viro
  2019-10-21 22:51     ` [PATCH v2] " Guillem Jover
  0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2019-10-21 20:15 UTC (permalink / raw)
  To: Jan Kara
  Cc: Guillem Jover, linux-aio, Christoph Hellwig, Jeff Moyer,
	Benjamin LaHaise, linux-fsdevel, linux-kernel

On Thu, Oct 17, 2019 at 03:48:00PM +0200, Jan Kara wrote:
> On Wed 21-08-19 05:38:20, Guillem Jover wrote:
> > This type is used to pass the sigset_t from userland to the kernel,
> > but it was using the kernel native pointer type for the member
> > representing the compat userland pointer to the userland sigset_t.
> > 
> > This messes up the layout, and makes the kernel eat up both the
> > userland pointer and the size members into the kernel pointer, and
> > then reads garbage into the kernel sigsetsize. Which makes the sigset_t
> > size consistency check fail, and consequently the syscall always
> > returns -EINVAL.
> > 
> > This breaks both libaio and strace on 32-bit userland running on 64-bit
> > kernels. And there are apparently no users in the wild of the current
> > broken layout (at least according to codesearch.debian.org and a brief
> > check over github.com search). So it looks safe to fix this directly
> > in the kernel, instead of either letting userland deal with this
> > permanently with the additional overhead or trying to make the syscall
> > infer what layout userland used, even though this is also being worked
> > around in libaio to temporarily cope with kernels that have not yet
> > been fixed.
> > 
> > We use a proper compat_uptr_t instead of a compat_sigset_t pointer.
> > 
> > Fixes: 7a074e96 ("aio: implement io_pgetevents")
> > Signed-off-by: Guillem Jover <guillem@hadrons.org>
> 
> This patch seems to have fallen through the cracks. Al?

Looks like - back then I assumed that Jens would've picked it...
Applied to #fixes...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
  2019-10-21 20:15   ` Al Viro
@ 2019-10-21 22:51     ` Guillem Jover
  2019-10-22 12:45       ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Guillem Jover @ 2019-10-21 22:51 UTC (permalink / raw)
  To: Al Viro
  Cc: Jan Kara, linux-aio, Christoph Hellwig, Jeff Moyer,
	Benjamin LaHaise, linux-fsdevel, linux-kernel

This type is used to pass the sigset_t from userland to the kernel,
but it was using the kernel native pointer type for the member
representing the compat userland pointer to the userland sigset_t.

This messes up the layout, and makes the kernel eat up both the
userland pointer and the size into the kernel pointer, and then
reads garbage into the kernel sigsetsize. Which makes the sigset_t
size consistency check fail, and consequently the syscall always
returns -EINVAL.

This breaks both libaio and strace on 32-bit userland running on 64-bit
kernels. And there are apparently no users in the wild of the current
broken layout (at least according to codesearch.debian.org and a brief
check over github.com search). So it looks safe to fix this directly
in the kernel, instead of either letting userland deal with this
permanently with the additional overhead or trying to make the syscall
infer what layout userland used, even though this is also being worked
around in libaio to temporarily cope with kernels that have not yet
been fixed.

We use a proper compat_uptr_t instead of a compat_sigset_t pointer.

Fixes: 7a074e96dee6 ("aio: implement io_pgetevents")
Signed-off-by: Guillem Jover <guillem@hadrons.org>
---
 fs/aio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 01e0fb9ae45a..0d9a559d488c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2179,7 +2179,7 @@ SYSCALL_DEFINE5(io_getevents_time32, __u32, ctx_id,
 #ifdef CONFIG_COMPAT
 
 struct __compat_aio_sigset {
-	compat_sigset_t __user	*sigmask;
+	compat_uptr_t		sigmask;
 	compat_size_t		sigsetsize;
 };
 
@@ -2193,7 +2193,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 		struct old_timespec32 __user *, timeout,
 		const struct __compat_aio_sigset __user *, usig)
 {
-	struct __compat_aio_sigset ksig = { NULL, };
+	struct __compat_aio_sigset ksig = { 0, };
 	struct timespec64 t;
 	bool interrupted;
 	int ret;
@@ -2204,7 +2204,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
 		return -EFAULT;
 
-	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
+	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
 	if (ret)
 		return ret;
 
@@ -2228,7 +2228,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
 		struct __kernel_timespec __user *, timeout,
 		const struct __compat_aio_sigset __user *, usig)
 {
-	struct __compat_aio_sigset ksig = { NULL, };
+	struct __compat_aio_sigset ksig = { 0, };
 	struct timespec64 t;
 	bool interrupted;
 	int ret;
@@ -2239,7 +2239,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
 	if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
 		return -EFAULT;
 
-	ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize);
+	ret = set_compat_user_sigmask(compat_ptr(ksig.sigmask), ksig.sigsetsize);
 	if (ret)
 		return ret;
 
-- 
2.24.0.rc0.303.g954a862665


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] aio: Fix io_pgetevents() struct __compat_aio_sigset layout
  2019-10-21 22:51     ` [PATCH v2] " Guillem Jover
@ 2019-10-22 12:45       ` Arnd Bergmann
  0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2019-10-22 12:45 UTC (permalink / raw)
  To: Guillem Jover
  Cc: Al Viro, Jan Kara, linux-aio, Christoph Hellwig, Jeff Moyer,
	Benjamin LaHaise, Linux FS-devel Mailing List, linux-kernel

On Tue, Oct 22, 2019 at 12:49 AM Guillem Jover <guillem@hadrons.org> wrote:
>
> This type is used to pass the sigset_t from userland to the kernel,
> but it was using the kernel native pointer type for the member
> representing the compat userland pointer to the userland sigset_t.
>
> This messes up the layout, and makes the kernel eat up both the
> userland pointer and the size into the kernel pointer, and then
> reads garbage into the kernel sigsetsize. Which makes the sigset_t
> size consistency check fail, and consequently the syscall always
> returns -EINVAL.
>
> This breaks both libaio and strace on 32-bit userland running on 64-bit
> kernels. And there are apparently no users in the wild of the current
> broken layout (at least according to codesearch.debian.org and a brief
> check over github.com search). So it looks safe to fix this directly
> in the kernel, instead of either letting userland deal with this
> permanently with the additional overhead or trying to make the syscall
> infer what layout userland used, even though this is also being worked
> around in libaio to temporarily cope with kernels that have not yet
> been fixed.
>
> We use a proper compat_uptr_t instead of a compat_sigset_t pointer.
>
> Fixes: 7a074e96dee6 ("aio: implement io_pgetevents")
> Signed-off-by: Guillem Jover <guillem@hadrons.org>

When resending a patch that has already been reviewed, please
add the tags you received so they get picked up into the final
changeset as well:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Let's make sure this also gets added to stable kernels

Cc: <stable@vger.kernel.org> # v4.18+

Finally (if you like)
Reviewed-by: Arnd Bergmann <arnd@arndb.de>

     Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-10-22 12:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-21  3:38 [PATCH] aio: Fix io_pgetevents() struct __compat_aio_sigset layout Guillem Jover
2019-08-21 23:55 ` Christoph Hellwig
2019-08-22 17:53 ` Jeff Moyer
2019-10-17 13:48 ` Jan Kara
2019-10-21 20:15   ` Al Viro
2019-10-21 22:51     ` [PATCH v2] " Guillem Jover
2019-10-22 12:45       ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).