Re: [GIT PULL] io_uring fixes for 6.0-rc1

[Jens Axboe <axboe@kernel.dk>]

On 8/12/22 3:54 PM, Linus Torvalds wrote:
> On Fri, Aug 12, 2022 at 2:43 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I think was seeing others (I got hundreds of lines or errors), but now
>> that I've blown things away I can't recreate it. My allmodconfig build
>> just completed with no sign of the errors I saw earlier.
> 
> Oh, and immediately after sending that email,  I got the errors back.
> 
> Because of the randstruct issue, I did another "git clean" (to make
> sure the random seed was gone and recreated) and started a new
> allmodconfig build.
> 
> And now I see the error again.
> 
> It does seem to be only 'struct io_cmd_data', but since this seems to
> be about random layout, who knows. The "hundreds of lines" is because
> each error report ends up being something like 25 lines in size, so
> you don't need a lot of them to get lots and lots of lines.
> 
> The ones I see in my *current* build are all that
> 
>   496 |         BUILD_BUG_ON(cmd_sz > sizeof(struct io_cmd_data));
> 
> add there's apparently six of them (so the "hundreds of lines" was
> apparently "only" 150 lines of errors), here's the concise "inlined
> from" info:
> 
>     inlined from ?io_prep_rw? at io_uring/rw.c:38:21:
>     inlined from ?__io_import_iovec? at io_uring/rw.c:353:21,
>     inlined from ?io_import_iovec? at io_uring/rw.c:406:11,
>     inlined from ?io_rw_prep_async? at io_uring/rw.c:538:8,
>     inlined from ?io_readv_prep_async? at io_uring/rw.c:551:9:
>     inlined from ?io_read? at io_uring/rw.c:697:21:
>     inlined from ?io_write? at io_uring/rw.c:842:21:
>     inlined from ?io_do_iopoll? at io_uring/rw.c:997:22:
> 
> in case that helps.

Thanks it does - so it's just io_rw, and hence it's just kiocb that is
problematic because of that layout randomization. What we really want is
for:

struct io_cmd_data {
	struct file		*file;
	/* each command gets 56 bytes of data */
	__u8			data[56];
};

to be == max-of-any-request-type data. Which was 56 bytes before, io_rw
and a few others were at that size. But if kiocb changes in an unlucky
fashion and we get the u16 and int interspersed between different types,
then struct kiocb growns and then io_rw as a result. And then the
compile blows up.

The patch was obviously a good thing since it found this, as this
would've caused some weird breakage that would've been hard to reproduce
unless your own build ended up having kiocb be larger as well.

Question is what to do about it. I can't think of a good way to size
io_cmd_data appropriately. We can union an io_rw in there, since that's
the biggest one and I _think_ the only one that'd hit this due to the
randomization. If I'm wrong, then it'd break compile again obviously.

Or we can ensure that kiocb doesn't get re-organized such that we add
more holes/padding. But that's also kind of weird.

Ideally we'd have a compile time way to check all structs, but that
becomes unwieldy.

For that one suggestion, I suspect this will fix your issue. It's
obviously not a thing of beauty...

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 677a25d44d7f..c83dedeb44b9 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -481,14 +481,29 @@ struct io_cqe {
 	};
 };
 
+struct io_rw {
+	/* NOTE: kiocb has the file as the first member, so don't do it here */
+	struct kiocb			kiocb;
+	u64				addr;
+	u32				len;
+	rwf_t				flags;
+};
+
 /*
  * Each request type overlays its private data structure on top of this one.
- * They must not exceed this one in size.
+ * They must not exceed this one in size. We must ensure that this is big
+ * enough to hold any command type. Currently io_rw includes struct kiocb,
+ * which is marked as having a random layout for security reasons. This can
+ * cause it to grow in size if the layout ends up adding more holes or padding.
+ * Unionize io_cmd_data with io_rw to work-around this issue.
  */
 struct io_cmd_data {
-	struct file		*file;
-	/* each command gets 56 bytes of data */
-	__u8			data[56];
+	union {
+		struct file		*file;
+		/* each command gets 56 bytes of data */
+		__u8			data[56];
+	};
+	struct io_rw pad;
 };
 
 static inline void io_kiocb_cmd_sz_check(size_t cmd_sz)
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 1babd77da79c..1c3a5da9dcdc 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -20,14 +20,6 @@
 #include "rsrc.h"
 #include "rw.h"
 
-struct io_rw {
-	/* NOTE: kiocb has the file as the first member, so don't do it here */
-	struct kiocb			kiocb;
-	u64				addr;
-	u32				len;
-	rwf_t				flags;
-};
-
 static inline bool io_file_supports_nowait(struct io_kiocb *req)
 {
 	return req->flags & REQ_F_SUPPORT_NOWAIT;

-- 
Jens Axboe