Skip to contentSkip to conversation

io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

PATCH In review Eugene Syromiatnikov 3 participants 7 messages Jan 15, 2020 – Jan 20, 2020

Subsystems (2)
VFS io_uring
lore
Share & tools
lore

Eugene Syromiatnikov

author
#0 lore raw

PATCH 1/1 io_uring: fix compat for IORING_REGISTER_FILES_UPDATE

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(-)
fs/io_uring.c +3 -1
index 38b5405..677ef90 100644
@@ -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;
include/uapi/linux/io_uring.h +2 -1
index a3300e1..55cfcb7 100644
@@ -178,7 +178,8 @@ struct io_uring_params {
 
 struct io_uring_files_update {
 	__u32 offset;
-	__s32 *fds;
+	__u32 resv;
+	__aligned_u64 /* __s32 * */ fds;
 };
 
 #endif
Signature · 1 line
2.1.4
maintainer
#1 lore raw
Quoted 6 lines · 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.

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?

Signature · 1 line
Jens Axboe
author
#2 lore raw
Quoted 11 lines · Jens Axboe
Quoted 6 lines · 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.

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

Quoted 7 lines
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.

Quoted 3 lines
Signature · 1 line
Jens Axboe
maintainer
#3 lore raw
Quoted 18 lines · Eugene Syromiatnikov
Quoted 17 lines · Jens Axboe
Quoted 6 lines · 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.

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;
};
Quoted 8 lines
Quoted 1 line

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.

Signature · 1 line
Jens Axboe
author
#4 lore raw
Quoted 1 line · Jens Axboe

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.

#5 lore raw
In reply to Jens Axboe
Quoted 28 lines · Jens Axboe
Quoted 18 lines · Eugene Syromiatnikov
Quoted 17 lines · Jens Axboe
Quoted 6 lines · 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.

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;
};
Quoted 8 lines
Quoted 1 line

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.

Quoted 3 lines

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,

Signature · 1 line
ldv
maintainer
#6 lore raw
Quoted 29 lines · Dmitry V. Levin
Quoted 28 lines · Jens Axboe
Quoted 26 lines · Eugene Syromiatnikov
Quoted 11 lines · Jens Axboe
Quoted 6 lines · 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.

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

Quoted 7 lines
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.

Quoted 6 lines
Quoted 3 lines

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.

Signature · 1 line
Jens Axboe