rpcsec_tls: cleanup the rpctls_syscall()

[glebius]

Summary

With all the recent changes we don't need extra argument that specifies
what exactly the syscalls does, neither we need a copyout-able pointer,
just a pointer sized integer.

[markj]

sys/kern/syscalls.master

3252

What happens if one runs an old rpc.tlsservd with a new kernel?

[glebius]

sys/kern/syscalls.master

3252

The syscall will return EINVAL. I don't know if the syscall argument parsing layer will do that or it will actually enter sys_rpctls_syscall(). If the latter happens, the small value that previously meant a command would be used as socookie and looked up in the database of stored sockets, and of course that lookup would fail.

[brooks]

I'm having a hard time following the code in this stack of reviews. Do you have a public git branch somewhere we can look at it all in one place.

sys/kern/syscalls.master

3252

Argument "parsing" consists of casting an array of syscallarg_t's to the uap structure. There's no checking of any form including argument count. If socookie is used in e.g., a hash table looking up then the ops values won't ever resolve so that would be ok-ish.

That being said it sure looks like socookie is a kernel pointer passed by userspace which is unsafe (I'm also seeing it being stored in an int64_t which should not be done).

[glebius]

In D48649#1109950, @brooks wrote:

I'm having a hard time following the code in this stack of reviews. Do you have a public git branch somewhere we can look at it all in one place.

https://github.com/glebius/FreeBSD/tree/gss-netlink

[glebius]

sys/kern/syscalls.master

3252

That being said it sure looks like socookie is a kernel pointer passed by userspace which is unsafe

It is safe cause kernel doesn't try to dereference it. It uses it as unique key lookup value. All unsafeness here is only leaking where socket located in kernel memory, but this is already reported by e.g. netstat(1) and even without a privilege check.

(I'm also seeing it being stored in an int64_t which should not be done).

Can you please advice any better approach? On the RPC side it must be a fixed size variable, hence it is uint64_t. But on the syscall side I made it uintptr_t that is supposed to fit into uint64_t on any modern machine.

[brooks]

sys/kern/syscalls.master

3252

Ah, if it's just an integer key then uint64_t (or maybe kvaddr_t to make it clear an address is wanted) seems fine and it would be best to use consistently rather than uintptr_t.

[glebius]

sys/kern/syscalls.master

3252

Ah, if it's just an integer key then uint64_t (or maybe kvaddr_t to make it clear an address is wanted) seems fine and it would be best to use consistently rather than uintptr_t.

kvaddr_t doesn't fit here, IMO, cause although we derive the number from a pointer, but we never plan to use it as a pointer or an address after that.

For the fixed size integer, my concern is how the code in rpctls_impl.c is going to work on 32-bit platforms?

arg.socookie = (uintptr_t)socookie;

So if I change this uintptr_t to uint64_t, I guess on 32-bits it is going to be just zero-extended, and everything should be fine. Is that correct?

[brooks]

sys/kern/syscalls.master

3252

Ah, if it's just an integer key then uint64_t (or maybe kvaddr_t to make it clear an address is wanted) seems fine and it would be best to use consistently rather than uintptr_t.

kvaddr_t doesn't fit here, IMO, cause although we derive the number from a pointer, but we never plan to use it as a pointer or an address after that.

kvaddr_t is for an address which is not a pointer. (Under the hood it's __uint64_t). I don't really mind either way, I just wanted to make sure it wasn't following the u64 pattern used in place of proper types in linux.

For the fixed size integer, my concern is how the code in rpctls_impl.c is going to work on 32-bit platforms?

arg.socookie = (uintptr_t)socookie;

So if I change this uintptr_t to uint64_t, I guess on 32-bits it is going to be just zero-extended, and everything should be fine. Is that correct?

It will be zero extended. When it's a syscall argument the high bits are explicitly zeroed when the register array is populated. With a cast the integer promotion rules apply also zeroing the high bits.

[rmacklem]

When I originally did this, I thought of using the socket pointer as a "unique"
indicator for it in userpsace (for the TCP socket the client/server is using).
I chose not to do so, because I was concerned that the kernel code might...

• close the socket
• create a new socket, that happens to have the same kernel address.

As such, I used a 64bit reference# that was incremented by 1 for each use,
assuming it would never wrap around, given it is 64bits.

If the socket pointer is guaranteed to not get closed during the upcall,
using a 64bit variant of the socket ptr should be safe, I think?

sys/kern/syscalls.master

3252

Just fyi, KTLS (and, therefore this TLS RPC stuff) only
works for 64bit arches, so I doubt it matters what a
32bit arch does with it.

[rmacklem]

Was changing the syscall and upcall arguments really necessary?
I thought it was the upcall RPC transport that needed to change.

Maybe the initial commit cycle could simply change the RPC upcall
transport to netlink and leave the rest as is. It would be a smaller change
and easier to isolate issues if only the RPC transport changes, I think?