Use-after free in `Op::connect`

[fasterthanlime]

I was wondering why connecting to IPv4 addresses sometimes worked.. but IPv6 doesn't. Sometimes I got an errno 97 (address family not supported), sometimes it was just hanging... and adding a couple dbg! statements made it work?

All of this smells a lot like memory unsafety.. and it is!

Here, a Connect struct is created, moving a SockAddr into it:

tokio-uring/src/driver/connect.rs

Lines 17 to 30 in 1c0a2bf

	Op::submit_with(
	Connect {
	fd: fd.clone(),
	socket_addr,
	},
	|connect| {
	opcode::Connect::new(
	types::Fd(connect.fd.raw_fd()),
	connect.socket_addr.as_ptr(),
	connect.socket_addr.len(),
	)
	.build()
	},
	)

Then, in submit_with, the data is moved into an Op struct, the callback is called to actually configure the SQE from the given data...

tokio-uring/src/driver/op.rs

Lines 74 to 78 in 1c0a2bf

	// Create the operation
	let mut op = Op::new(data, inner, inner_rc);
	// Configure the SQE
	let sqe = f(op.data.as_mut().unwrap()).user_data(op.index as _);

And then op is dropped (and the data with it). We now have an op in the submission queue that refers to some userspace address that's been freed (somewhere on the stack, presumably). This probably works well in tests because immediately after attempting to connect, it sleeps. But we now have the kernel using some memory after we've freed it.

A potential fix would be to move the data into the Lifecycle::Submitted variant - but that of course raises the question of: should it be boxed? Probably not right? So there's only so much data we can stuff in there - maybe connect is one of the rare ops that needs something like this, so maybe it should itself contain an enum?

At any rate, that explains the random failures I've been getting.

[fasterthanlime]

Note that the Accept op boxes the socketaddr before pushing an sqe that points to it:

tokio-uring/src/driver/accept.rs

Lines 15 to 18 in 1c0a2bf

	let socketaddr = Box::new((
	unsafe { std::mem::zeroed() },
	std::mem::size_of::<libc::sockaddr_storage>() as libc::socklen_t,
	));

I still don't think that's enough, but there's much fewer chances of that heap-allocated memory being overwritten by the time the kernel gets to it.

[fasterthanlime]

Ah, the accept situation is a bit different since it's an out pointer. I see what holds the data now - the op isn't dropped, it's returned. That's why when I simply tried boxing the connect address, it also fixed it. It wasn't luck (that the heap memory location wasn't getting overwritten), it was that even though I was moving the Op future around, the location of the connect address remained stable.

So, Boxing the address would fix the connect thing for good, but I still feel like having the "future handle" hold associated data is the wrong model. If one were to drop the connect/accept future before the operation completes, the kernel would be reading from / writing to invalid memory - same lifetime concerns as buffers.

[FrankReh]

Is it the accept op or the connection op that you think is dropping the fd too early?

I don't know all the details of the latest Completable design but I hadn't had problems with the earlier one. I thought the code paths kept the fd in a SharedFd that only got dropped when the Op was dropped and the operation had completed per a cqe entry.

Oh, not the fd, the address?

The Connect struct in connect.rs takes ownership of the SockAddr. How is that being dropped before the Op is dropped? I don't see unsafe being used anywhere around the connect.socket_addr so I'm wondering about the address of the sock address being passed to the kernel.

[fasterthanlime]

Oh, not the fd, the address?

Exactly. As things stand now, both accept and connect are unsafe:

• accept cannot be dropped, or whenever the kernel does accept a socket it'll write the local_addr in a freed heap memory location
• connect cannot be dropped (or kernel reads from a freed location) but it also cannot be moved, which moves the location of the SockAddr and has the kernel read garbage, too.

That model is flawed imho - both the inputs AND the outputs must be kept in the slab so they never move until the operation is successfully completed OR successfully cancelled. The future handle must be just that - a handle.

[FrankReh]

What version of kernel are you testing on? For sometime, the kernel driver folks have said the data only needs to be good
until the 'enter' call is made, not the whole time the operation is in flight.

[FrankReh]

• accept cannot be dropped, or whenever the kernel does accept a socket it'll write the local_addr in a freed heap memory location

Confused by this. The kernel doesn't write to any of our data, it returns a result and some flags bits. Do you mean you think the kernel has kept the original SockAddr pointers during its accept? I didn't see that documented anywhere in their man pages but I've never tried looking at their kernel code.

[fasterthanlime]

What version of kernel are you testing on?

This is on:

❯ uname -r
5.15.0-48-generic

For sometime, the kernel driver folks have said the data only needs to be good
until the 'enter' call is made, not the whole time the operation is in flight.

Submissions are batched: if the queue isn't full, submit_with just does sq.push, which does not enter. (But you're right that this means the inputs can probably live right until enter). I literally started playing with io_uring today, so I'm still not sure about anything, but - isn't there also a mode where submitting ops doesn't require calling enter at all?

Confused by this. The kernel doesn't write to any of our data, it returns a result and some flags bits. Do you mean you think the kernel has kept the original SockAddr pointers during its accept? I didn't see that documented anywhere in their man pages but I've never tried looking at their kernel code.

That is what I think is happening, yes - we're giving the kernel a place to store the local_addr for the accepted connection:

https://github.com/tokio-rs/io-uring/blob/aae166577e307fd9f39d1d0401234b78b372ebb0/src/opcode.rs#L558-L581

[Noah-Kennedy]

Accept is fine because on drop the state gets dropped into the driver (Lifecycle::Ignored).

[fasterthanlime]

Accept is fine because on drop the state gets dropped into the driver (Lifecycle::Ignored).

That's true! That means this issue was closed by #126.