IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
@ 2020-01-15 16:35 Eugene Syromiatnikov
  2020-01-15 16:41 ` Jens Axboe
  0 siblings, 1 reply; 7+ messages in thread
From: Eugene Syromiatnikov @ 2020-01-15 16:35 UTC (permalink / raw)
  To: linux-fsdevel, io-uring, Alexander Viro, Jens Axboe
  Cc: linux-kernel, Jeff Moyer, Dmitry V. Levin

fds field of struct io_uring_files_update is problematic with regards
to compat user space, as pointer size is different in 32-bit, 32-on-64-bit,
and 64-bit user space.  In order to avoid custom handling of compat in
the syscall implementation, make fds __u64 and use u64_to_user_ptr in
order to retrieve it.  Also, align the field naturally and check that
no garbage is passed there.

Fixes: c3a31e605620c279 ("io_uring: add support for IORING_REGISTER_FILES_UPDATE")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
 fs/io_uring.c                 | 4 +++-
 include/uapi/linux/io_uring.h | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 38b5405..677ef90 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4445,13 +4445,15 @@ static int io_sqe_files_update(struct io_ring_ctx *ctx, void __user *arg,
 		return -EINVAL;
 	if (copy_from_user(&up, arg, sizeof(up)))
 		return -EFAULT;
+	if (up.resv)
+		return -EINVAL;
 	if (check_add_overflow(up.offset, nr_args, &done))
 		return -EOVERFLOW;
 	if (done > ctx->nr_user_files)
 		return -EINVAL;
 
 	done = 0;
-	fds = (__s32 __user *) up.fds;
+	fds = u64_to_user_ptr(up.fds);
 	while (nr_args) {
 		struct fixed_file_table *table;
 		unsigned index;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index a3300e1..55cfcb7 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -178,7 +178,8 @@ struct io_uring_params {
 
 struct io_uring_files_update {
 	__u32 offset;
-	__s32 *fds;
+	__u32 resv;
+	__aligned_u64 /* __s32 * */ fds;
 };
 
 #endif
-- 
2.1.4


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

* Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
  2020-01-15 16:35 [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE Eugene Syromiatnikov
@ 2020-01-15 16:41 ` Jens Axboe
  2020-01-15 16:50   ` Eugene Syromiatnikov
  0 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2020-01-15 16:41 UTC (permalink / raw)
  To: Eugene Syromiatnikov, linux-fsdevel, io-uring, Alexander Viro
  Cc: linux-kernel, Jeff Moyer, Dmitry V. Levin

On 1/15/20 9:35 AM, Eugene Syromiatnikov wrote:
> fds field of struct io_uring_files_update is problematic with regards
> to compat user space, as pointer size is different in 32-bit, 32-on-64-bit,
> and 64-bit user space.  In order to avoid custom handling of compat in
> the syscall implementation, make fds __u64 and use u64_to_user_ptr in
> order to retrieve it.  Also, align the field naturally and check that
> no garbage is passed there.

Good point, it's an s32 pointer so won't align nicely. But how about
just having it be:

struct io_uring_files_update {
	__u32 offset;
	__u32 resv;
	__s32 *fds;
};

which should align nicely on both 32 and 64-bit?

-- 
Jens Axboe


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

* Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
  2020-01-15 16:41 ` Jens Axboe
@ 2020-01-15 16:50   ` Eugene Syromiatnikov
  2020-01-15 16:53     ` Jens Axboe
  0 siblings, 1 reply; 7+ messages in thread
From: Eugene Syromiatnikov @ 2020-01-15 16:50 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-fsdevel, io-uring, Alexander Viro, linux-kernel,
	Jeff Moyer, Dmitry V. Levin

On Wed, Jan 15, 2020 at 09:41:58AM -0700, Jens Axboe wrote:
> On 1/15/20 9:35 AM, Eugene Syromiatnikov wrote:
> > fds field of struct io_uring_files_update is problematic with regards
> > to compat user space, as pointer size is different in 32-bit, 32-on-64-bit,
> > and 64-bit user space.  In order to avoid custom handling of compat in
> > the syscall implementation, make fds __u64 and use u64_to_user_ptr in
> > order to retrieve it.  Also, align the field naturally and check that
> > no garbage is passed there.
> 
> Good point, it's an s32 pointer so won't align nicely. But how about
> just having it be:
> 
> struct io_uring_files_update {
> 	__u32 offset;
> 	__u32 resv;
> 	__s32 *fds;
> };
> 
> which should align nicely on both 32 and 64-bit?

The issue is that 32-bit user space would pass a 12-byte structure with
a 4-byte pointer in it to the 64-bit kernel, that, in turn, would treat it
as a 8-byte value (which might sometimes work on little-endian architectures,
if there are happen to be zeroes after the pointer, but will be always broken
on big-endian ones). __u64 is used in order to avoid special compat wrapper;
see, for example, __u64 usage in btrfs or BPF for similar purposes.

> -- 
> Jens Axboe
> 


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

* Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
  2020-01-15 16:50   ` Eugene Syromiatnikov
@ 2020-01-15 16:53     ` Jens Axboe
  2020-01-15 16:59       ` Eugene Syromiatnikov
  2020-01-20 23:51       ` Dmitry V. Levin
  0 siblings, 2 replies; 7+ messages in thread
From: Jens Axboe @ 2020-01-15 16:53 UTC (permalink / raw)
  To: Eugene Syromiatnikov
  Cc: linux-fsdevel, io-uring, Alexander Viro, linux-kernel,
	Jeff Moyer, Dmitry V. Levin

On 1/15/20 9:50 AM, Eugene Syromiatnikov wrote:
> On Wed, Jan 15, 2020 at 09:41:58AM -0700, Jens Axboe wrote:
>> On 1/15/20 9:35 AM, Eugene Syromiatnikov wrote:
>>> fds field of struct io_uring_files_update is problematic with regards
>>> to compat user space, as pointer size is different in 32-bit, 32-on-64-bit,
>>> and 64-bit user space.  In order to avoid custom handling of compat in
>>> the syscall implementation, make fds __u64 and use u64_to_user_ptr in
>>> order to retrieve it.  Also, align the field naturally and check that
>>> no garbage is passed there.
>>
>> Good point, it's an s32 pointer so won't align nicely. But how about
>> just having it be:
>>
>> struct io_uring_files_update {
>> 	__u32 offset;
>> 	__u32 resv;
>> 	__s32 *fds;
>> };
>>
>> which should align nicely on both 32 and 64-bit?
> 
> The issue is that 32-bit user space would pass a 12-byte structure with
> a 4-byte pointer in it to the 64-bit kernel, that, in turn, would treat it
> as a 8-byte value (which might sometimes work on little-endian architectures,
> if there are happen to be zeroes after the pointer, but will be always broken
> on big-endian ones). __u64 is used in order to avoid special compat wrapper;
> see, for example, __u64 usage in btrfs or BPF for similar purposes.

Ah yes, I'm an idiot, apparently not enough coffee yet. We'd need it in
a union for this to work. I'll just go with yours, it'll work just fine.
I will fold it in, I need to make some updates and rebase anyway.

-- 
Jens Axboe


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

* Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
  2020-01-15 16:53     ` Jens Axboe
@ 2020-01-15 16:59       ` Eugene Syromiatnikov
  2020-01-20 23:51       ` Dmitry V. Levin
  1 sibling, 0 replies; 7+ messages in thread
From: Eugene Syromiatnikov @ 2020-01-15 16:59 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-fsdevel, io-uring, Alexander Viro, linux-kernel,
	Jeff Moyer, Dmitry V. Levin

On Wed, Jan 15, 2020 at 09:53:27AM -0700, Jens Axboe wrote:
> We'd need it in a union for this to work.

Note that union usage may be a bit problematic, as it may lead to difference
in behaviour (and possible subtle bugs, as a result) between native 32-bit
kernel and 64-bit one in compat mode due to the fact that u64_to_user_ptr
doesn't check higher 32 bits on 32 bit kernels; it is mostly ignored
in the case of plain __u64 usage, as it is less likely to pass garbage
in the higher 32 bits in that case, but the issue, nevertheless, stands,
so I'd propose to check these bits in case the union approach
is implemented.


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

* Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
  2020-01-15 16:53     ` Jens Axboe
  2020-01-15 16:59       ` Eugene Syromiatnikov
@ 2020-01-20 23:51       ` Dmitry V. Levin
  2020-01-20 23:54         ` Jens Axboe
  1 sibling, 1 reply; 7+ messages in thread
From: Dmitry V. Levin @ 2020-01-20 23:51 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Eugene Syromiatnikov, linux-fsdevel, io-uring, Alexander Viro,
	linux-kernel, Jeff Moyer

On Wed, Jan 15, 2020 at 09:53:27AM -0700, Jens Axboe wrote:
> On 1/15/20 9:50 AM, Eugene Syromiatnikov wrote:
> > On Wed, Jan 15, 2020 at 09:41:58AM -0700, Jens Axboe wrote:
> >> On 1/15/20 9:35 AM, Eugene Syromiatnikov wrote:
> >>> fds field of struct io_uring_files_update is problematic with regards
> >>> to compat user space, as pointer size is different in 32-bit, 32-on-64-bit,
> >>> and 64-bit user space.  In order to avoid custom handling of compat in
> >>> the syscall implementation, make fds __u64 and use u64_to_user_ptr in
> >>> order to retrieve it.  Also, align the field naturally and check that
> >>> no garbage is passed there.
> >>
> >> Good point, it's an s32 pointer so won't align nicely. But how about
> >> just having it be:
> >>
> >> struct io_uring_files_update {
> >> 	__u32 offset;
> >> 	__u32 resv;
> >> 	__s32 *fds;
> >> };
> >>
> >> which should align nicely on both 32 and 64-bit?
> > 
> > The issue is that 32-bit user space would pass a 12-byte structure with
> > a 4-byte pointer in it to the 64-bit kernel, that, in turn, would treat it
> > as a 8-byte value (which might sometimes work on little-endian architectures,
> > if there are happen to be zeroes after the pointer, but will be always broken
> > on big-endian ones). __u64 is used in order to avoid special compat wrapper;
> > see, for example, __u64 usage in btrfs or BPF for similar purposes.
> 
> Ah yes, I'm an idiot, apparently not enough coffee yet. We'd need it in
> a union for this to work. I'll just go with yours, it'll work just fine.
> I will fold it in, I need to make some updates and rebase anyway.

I see the patch has missed v5.5-rc7.
Jens, please make sure a fix is merged before v5.5 is out.
Thanks,


-- 
ldv

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

* Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE
  2020-01-20 23:51       ` Dmitry V. Levin
@ 2020-01-20 23:54         ` Jens Axboe
  0 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2020-01-20 23:54 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Eugene Syromiatnikov, linux-fsdevel, io-uring, Alexander Viro,
	linux-kernel, Jeff Moyer

On 1/20/20 4:51 PM, Dmitry V. Levin wrote:
> On Wed, Jan 15, 2020 at 09:53:27AM -0700, Jens Axboe wrote:
>> On 1/15/20 9:50 AM, Eugene Syromiatnikov wrote:
>>> On Wed, Jan 15, 2020 at 09:41:58AM -0700, Jens Axboe wrote:
>>>> On 1/15/20 9:35 AM, Eugene Syromiatnikov wrote:
>>>>> fds field of struct io_uring_files_update is problematic with regards
>>>>> to compat user space, as pointer size is different in 32-bit, 32-on-64-bit,
>>>>> and 64-bit user space.  In order to avoid custom handling of compat in
>>>>> the syscall implementation, make fds __u64 and use u64_to_user_ptr in
>>>>> order to retrieve it.  Also, align the field naturally and check that
>>>>> no garbage is passed there.
>>>>
>>>> Good point, it's an s32 pointer so won't align nicely. But how about
>>>> just having it be:
>>>>
>>>> struct io_uring_files_update {
>>>> 	__u32 offset;
>>>> 	__u32 resv;
>>>> 	__s32 *fds;
>>>> };
>>>>
>>>> which should align nicely on both 32 and 64-bit?
>>>
>>> The issue is that 32-bit user space would pass a 12-byte structure with
>>> a 4-byte pointer in it to the 64-bit kernel, that, in turn, would treat it
>>> as a 8-byte value (which might sometimes work on little-endian architectures,
>>> if there are happen to be zeroes after the pointer, but will be always broken
>>> on big-endian ones). __u64 is used in order to avoid special compat wrapper;
>>> see, for example, __u64 usage in btrfs or BPF for similar purposes.
>>
>> Ah yes, I'm an idiot, apparently not enough coffee yet. We'd need it in
>> a union for this to work. I'll just go with yours, it'll work just fine.
>> I will fold it in, I need to make some updates and rebase anyway.
> 
> I see the patch has missed v5.5-rc7.
> Jens, please make sure a fix is merged before v5.5 is out.

Ah shoot, I actually thought I added it for 5.6 only, but you are right,
it's in 5.5-rc as well. I'll ship a patch this week for 5.5.

-- 
Jens Axboe


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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 16:35 [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE Eugene Syromiatnikov
2020-01-15 16:41 ` Jens Axboe
2020-01-15 16:50   ` Eugene Syromiatnikov
2020-01-15 16:53     ` Jens Axboe
2020-01-15 16:59       ` Eugene Syromiatnikov
2020-01-20 23:51       ` Dmitry V. Levin
2020-01-20 23:54         ` Jens Axboe

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git