Re: [PATCH net v7 00/11] rxrpc: Fix CHALLENGE packet handling

[David Howells <dhowells@redhat.com>]

Jakub Kicinski <kuba@kernel.org> wrote:

> FWIW I had to kick off clashiko manually because sparse false-positives
> on one of the patches. It should finish in 30min or so. I'm signing off
> for the day, so please TAL if you can:
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260812110129.979970-6-dhowells@redhat.com
> and let us know if we should ship v7 as is or you want to tweak..

Okay, there are only a couple of things that might merit producing a v8:

 (1) There's a missing key_put() in afs_open_socket()'s error path.

 (2) I should probably require READ permission on the key holding the appdata
     provided by usespace through RXRPC_RESPONSE_APPDATA rather than SEARCH
     permission to prevent this being used to pull the data out of keys that
     can't otherwise read directly with keyctl().

I can fix both of these with follow-up single line fix patches or (2) could
be fixed in place at the point of application:

--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -640,7 +640,7 @@ static int rxrpc_sendmsg_cmsg(struct msghdr *msg, struct rxrpc_send_params *p)
 			if (p->call.app_data)
 				return -EINVAL;
 			key_id = *(key_serial_t *)CMSG_DATA(cmsg);
-			key = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
+			key = lookup_user_key(key_id, 0, KEY_NEED_READ);
 			if (IS_ERR(key))
 				return PTR_ERR(key);
 			if (key_ref_to_ptr(key)->type != &key_type_user &&

Everything else, I think, can be safely deferred.  I've discussed the points
raised below.

David
---
=== Patch 1

"This isn't a bug introduced by this patch, but should the kernel-doc for the
exported rxrpc_kernel_send_data() be updated in the same change?" - I can
address doc updates in an additonal patch.

"The two in-tree callers already disagree on how success is encoded:
fs/afs/rxrpc.c:afs_send_empty_reply() switches on "case 0:" while
afs_send_simple_reply() tests "n >= 0"." - That's probably worth fixing, but
it'd be an AFS patch and isn't relevant to this patchset.

=== Patch 2

"Is there a reachable case where len differs from
iov_iter_count(&msg->msg_iter) on entry to rxrpc_send_data()?" - This really
applies to all implementations of ->sendmsg(), not just rxrpc.  In theory,
maybe; but in practice I don't think so.  I did spend some time looking if we
could eliminate the len argument entirely, but that doesn't need dealing with
here.  I've made the assumption that "len" controls how much we want to write,
particularly for the purpose of marking the last packet, and if msg_iter is
short, then we return a short send (if we've already filled a txbuf) or
-EFAULT (if we haven't) and don't mark last packet.  If there's more in
msg_iter, we just ignore the excess.

I can create an additional patch to fix the doc.

=== Patch 3

"The changelog only describes rxrpc_send_data(), and doesn't mention this
fs/afs/rxrpc.c hunk at all." - That's not actually true; it even quotes the
mention about converting to retry loops.

"Are the two lines immediately following now stale?" - True, but I can remove
the stale lines with a followup patch.

"Can -ENOMEM here leave a partially encrypted txbuf that is later encrypted a
second time?" - The assumption is that if ENOMEM occurs, we haven't tried to
encrypt the buffer yet.  Even if a confounder has been inserted, that's not a
problem as it's overwriting the specific bit of buffer reserved for it.  A new
confounder can just be written over it.  skcipher shouldn't go down the slow
path as the buffer should be correctly aligned and encryption is done in
place.

"The second half of the concern is that the retry does not always re-copy the
plaintext." - This shouldn't matter.  If the packet is successfully encrypted,
then txb/call->tx_pending should be cleared until we go back to the top of the
loop and allocate a new txbuf.  As previously mentioned, the assumption is
that once we actually start encrypting, we should not fail with ENOMEM.

"Does this condition depend on an earlier patch in the series that isn't cc'd
to stable?" - An oversight, but I'm not sure it matters enough to respin.

=== Patch 4

"Can this store ever move a non-NULL txb?" - Good point; txb must be NULL,
otherwise we wouldn't come down the wait_for_space branch.  But it doesn't
really need fixing; it just writes NULL twice to the same place under lock.

"The new out_nolock does "return copied ?: ret", so with copied > 0 the
negative value from the wait is discarded." - This change is correct.
Possibly it should have been mentioned in the changelog as a change of
behaviour.

"Also, the trace prints ret while the function returns "copied ?: ret"" - It
doesn't really matter, but I probably want to see the error that caused the
return there.

=== Patch 5

"This isn't a bug introduced by this patch, but since the code is being
relocated here it may be worth a look: should these nested sections use
spin_lock_irqsave()/spin_unlock_irqrestore() instead?" - No point as IRQs are
known to be enabled.

"I could not find an IRQ-context acquirer of either call->recvmsg_queue.lock
or rx->recvmsg_lock, so this looks like a fragility rather than a demonstrable
deadlock today." - The problem is that the app thread can otherwise hold up
the I/O thread, particularly if realtime is involved.

"Further notifications are suppressed by putting recvmsg_link on a dummy
queue." - The comment needs updating, but that can be done with a follow-up
patch.

"Should the prototype move too?" - Yeah.  A follow-up patch can do that.

=== Patch 6

Nothing mentioned.

=== Patch 7

"Should this patch carry a Fixes: tag?  It is cc'd to stable but does not name
the defect it fixes, nor the user-visible symptom in the AF_RXRPC challenge
response path." - With regard to the keys patch, that's not technically a fix,
but a prerequisite.

"Does the comment block just above struct user_key_payload need updating?" -
Yeah, but that can be a follow-up patch.

"Is the comment on put_user_key_payload() accurate?" - Ditto.

=== Patch 8

"This isn't a bug introduced by this patch, but the new length calculation
here differs from the pre-existing one in afs_create_yfs_cm_token() in the
same file, which reads:" - Yeah, and also, as noted, the code is removed.

"Is anything reading server->yfs_rxgk_appdata as of this commit?" - See the
next patch.

"Can a cached fileserver record end up being used for RxGK calls without ever
getting appdata created?" - Yes, this can happen to probe calls for the
moment.  That can be fixed, but I think separately as it's purely work in
fs/afs/.

"and -ENOPKG is user-influenced, since rxrpc_preparse_xdr_yfs_rxgk() only
range-checks the token enctype rather than looking it up with
crypto_krb5_find_enctype()." - But it creates a callback key of the same type
as the key the user provides to make FS calls.  We don't get a CHALLENGE
packet until we have encrypted a DATA packet and sent it, in which case
-ENOPKG would have already happened.

If afs_create_token_key() fails, we can still do unencrypted Cache Manager
service comms, which is why I don't make it a fatal error.

"Does the address-update path silently swallow these new failures?" - Um,
that's really about talking to the VL server, not the FS server.  The VL
server doesn't involve interaction eith the CM service (that's just the
3rd-party change notification channel from the FS server).

Possibly there is a further bug here, but it's an AFS bug, not an AF_RXRPC
bug if so.

"Should this pick the same token that rxrpc will actually use for the
connection, rather than always the first one?" - This is actually a
pre-existing issue and needs more work.

=== Patch 9

"Should Documentation/networking/rxrpc.rst be updated alongside this?" - That
actually needs a more comprehensive update and can be done separately.

"RXRPC_SUPPORTED_CMSG sees type 20 as available with nothing in the
documentation describing it, its key_serial_t payload, or the restriction to
key_type_user/key_type_logon keys." - Yeah.  I need to add that, but it can be
done in a follow-up patch.

"Should this case also validate p->command, or that the sendmsg() is going to
create a new call?" - The parameter is just ignored if not creating a call.  I
suppose I could add a check that a call is being created - but that can be
done in a separate patch.

=== Patch 10

"Can this path disclose the payload of a key the caller is not permitted to
read?" - That's a good point and needs READ permission, not SEARCH permission.
That can be followed up with a quick patch, I think, but might need the patch
respinning.

=== Patch 11

"does afs_open_socket() leak the key created by afs_create_token_key()?" -
Yeah, that needs a fix.

"Should the rxrpc_abort_reason enum entry go too, in the same way as these?
EM(rxrpc_abort_response_sendmsg, "resp-sendmsg")" - That can be done in a
follow-up patch.

"With RXRPC_CHALLENGED gone from this enum, are the structs describing it
meant to stay in the same header?" - Those can be removed in a follow-up
patch.

"#define RXRPC_MANAGE_RESPONSE" - Ditto.

"setsockopt(SOL_RXRPC, RXRPC_MANAGE_RESPONSE)" - The stub handler for that can
be removed in a follow up patch.