* [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 <[email protected]> --- 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 related [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, other threads:[~2020-01-20 23:56 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox