[PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Eugene Syromiatnikov]

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

Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Jens Axboe]

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

Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Eugene Syromiatnikov]

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
> 

Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Jens Axboe]

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

Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Eugene Syromiatnikov]

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.

Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Dmitry V. Levin]

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

Re: [PATCH] io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

[Jens Axboe]

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