From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 90E3F1684A4; Sat, 21 Mar 2026 01:47:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774057652; cv=none; b=TKPlDMLP1B23dbNyJ02frwRIUtxl0me3MidpgDi6NgWRjr48mxGguL1cOkk7Mct4vjWBjD5RYpai7cTdBq5LZPdNiL8IJ1SX9UGZBhODgKscsVlUODQRLJUzhXbiokaQ0vk8MTy9JzIMDpA+1lAw414zqxMJTJHdWi0M+5i1lSY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774057652; c=relaxed/simple; bh=OXHU1TAckiv5f6PHmo0z+bLguWAVdik7LFHVuCVfWcE=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=DEtbRVtNAzMPXD3f6vOifV89hcVmbEul8fdaOebZBQQzElAlNZgrw2ok1gSyqbQHK2gGznpnz8mkRX8Ig68bnZUAzbDEkLKOwUT2qE0uESd6ZeURsZtXtzzskmNolynYbvZYOgoQ93k1jZcti4/L0cJ315//RFzMmWeSFZJsRAg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=lLvc+3Rd; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="lLvc+3Rd" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 72D28C4CEF7; Sat, 21 Mar 2026 01:47:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1774057652; bh=OXHU1TAckiv5f6PHmo0z+bLguWAVdik7LFHVuCVfWcE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=lLvc+3Rd2wU6fwFEF5Bb8pwt7ci4uic84IYNxWOWPQRBdGXuMtDYkXF/HPtNPN2Ga 1YBi9r0BGP07AR7iKviBnHgl8nnyHGXNoKg9Oy0dU5rr5bcFkyiQ2dPnJup5wvdTwF ufSKmhnNvPadjKr6wsQtVojbFGQdwKR77AGSskWX6AnsX5fTj0Yr3P42/GhJkAQAep P8SOFO95ZnR0CGZP/jFby2kgNhHB1fmmMGuYi4WSv2DyWwKki9krk9eubJi2KGamlK IqMbJhtEBRDY5Gds0NLHlEsaHt2VQtHhJ131dpZbnpQwtBcMhZwBkeIFu9J/mzPLPR FjvNcGATQ581Q== From: Jakub Kicinski To: dhowells@redhat.com Cc: Jakub Kicinski , netdev@vger.kernel.org, linux-afs@lists.infradead.org, horms@kernel.org, alok.a.tiwari@oracle.com, davem@davemloft.net, marc.dionne@auristor.com, linux-kernel@vger.kernel.org, security@1seal.org, stable@kernel.org, pabeni@redhat.com, anderson@allelesecurity.com, jaltman@auristor.com, edumazet@google.com Subject: Re: [PATCH net 2/5] rxrpc: Fix use of wrong skb when comparing queued RESP challenge serial Date: Fri, 20 Mar 2026 18:45:44 -0700 Message-ID: <20260321014543.3892478-2-kuba@kernel.org> X-Mailer: git-send-email 2.53.0 In-Reply-To: <20260319150150.4189381-3-dhowells@redhat.com> References: <20260319150150.4189381-3-dhowells@redhat.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Hi David! The review under --- is AI generated. It looks legit to me. The Google reviewer also points out preexiting problems that may be worth looking at: https://sashiko.dev/#/patchset/20260319150150.4189381-1-dhowells%40redhat.com For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- rxrpc: Fix use of wrong skb when comparing queued RESP challenge serial In rxrpc_post_response(), the code should be comparing the challenge serial number from the cached response before deciding to switch to a newer response, but looks at the newer packet private data instead, rendering the comparison always false. Fix this by switching to look at the older packet. > diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c > index 98ad9b51ca2c..2c27f47951f9 100644 > --- a/net/rxrpc/conn_event.c > +++ b/net/rxrpc/conn_event.c > @@ -557,7 +557,7 @@ void rxrpc_post_response(struct rxrpc_connection *conn, struct sk_buff *skb) > spin_lock_irq(&local->lock); > old = conn->tx_response; > if (old) { > - struct rxrpc_skb_priv *osp = rxrpc_skb(skb); > + struct rxrpc_skb_priv *osp = rxrpc_skb(old); > > /* Always go with the response to the most recent challenge. */ > if (after(sp->resp.challenge_serial, osp->resp.challenge_serial)) > conn->tx_response = old; ^^^^ Does this have the logic inverted? The comment says "Always go with the response to the most recent challenge" and the condition checks if the new response has a more recent serial number: if (after(sp->resp.challenge_serial, osp->resp.challenge_serial)) When this is TRUE (new serial is after old serial), the code keeps the OLD response by setting conn->tx_response = old. Shouldn't it keep the NEW response instead? Something like: if (after(sp->resp.challenge_serial, osp->resp.challenge_serial)) conn->tx_response = skb; This would cause the connection to respond to outdated challenges rather than the most recent one. > else > old = skb; When a new response is rejected here (because the old response has a more recent challenge serial), what happens to the rejected packet? The function assigns it to the local variable 'old' but never frees it. Looking at the callers: - rxkad_respond_to_challenge() in rxkad.c sets response = NULL after calling this function - rxgk_construct_response() in rxgk.c does the same This suggests ownership transfers to rxrpc_post_response(). Should the rejected packet be freed with rxrpc_free_skb() before returning? > } else { > conn->tx_response = skb; > } > spin_unlock_irq(&local->lock); > rxrpc_poke_conn(conn, rxrpc_conn_get_poke_response); > } -- pw-bot: cr