Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Daniel Jordan <daniel.m.jordan@oracle.com>
To:     Boris Pismenny <borisp@nvidia.com>,
        John Fastabend <john.fastabend@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jakub Kicinski <kuba@kernel.org>,
        David Miller <davem@davemloft.net>,
        Dave Watson <davejwatson@fb.com>,
        Vakul Garg <vakul.garg@nxp.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Daniel Jordan <daniel.m.jordan@oracle.com>
Subject: [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls
Date:   Thu, 21 Oct 2021 14:30:43 -0400
Message-Id: <20211021183043.837139-1-daniel.m.jordan@oracle.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?+Wx6lsA8rnGSXoL4ueBXPkD6cgNaqHkPqEF/EDK7Ts65yTCVEgPpSzTlpQYk?=
 =?us-ascii?Q?o6DdD0Bjv1M5fHHbkfLRjamYzaiOs+P7a9+schFQe4KBQZ4wBzyYSjZK6tdh?=
 =?us-ascii?Q?GOtK7k4cg5uVGoNGaa2bZlMXMmRSkBo5HxNy/cETnTfYyL7EW+oQ3QxHQdLx?=
 =?us-ascii?Q?Wt5A2xFsd+JvfrRwl5iY/ZDDj2i0jRZDXCXeoioF+HnLBozH2incP4AI209P?=
 =?us-ascii?Q?iz/ibzH+0/DfSjeKYsX53vaGKDfL3hyTbU2YSBbrsqwf6kNlPIKwQMSCsrTu?=
 =?us-ascii?Q?IJC+wBTwVR55ebvIFQ3Lhyeczp6IRTEpkhjIhJvJOWCp6uTh/KZjGNeKicTQ?=
 =?us-ascii?Q?xA3yR+hD+w909hYD5QOBwfMruytm4bU89e7G1G01pPmBzI9mfB/bJrE+mK6r?=
 =?us-ascii?Q?QkVEZHcbN0Ap72Du+HPIbKG1vFCoa8D2RFPph+9dN6yj8fZphdYedF8KE2dR?=
 =?us-ascii?Q?jo+7QrNuXL3VQcKrFEP6uFi0gZEG1WzruVT2b1ZPp81t1c/X33GR2LwRbz/V?=
 =?us-ascii?Q?pPakHA+FBXvwHAJGBeIVeemPqPAwFI/uorGxDZxQyTTIbWgc8YJ80vd39aGz?=
 =?us-ascii?Q?b25KeycnGCvF+bTzEkIIWAlW6CViphJQugrsV8EqUTkHHcanvcyly9v2dOsY?=
 =?us-ascii?Q?67H7uk14jqsE/pzfIU0r+QooXjAeSPsr4gSJ0x2k8uzDOyfgI+jxz49Rx0lp?=
 =?us-ascii?Q?anhbivwUZ45LqaVNKwCFhQrMl9bpkH0tDpR5PVHQaWrtp3+OkRQtxQqE+I8Y?=
 =?us-ascii?Q?6Tdr6FnVu56iGPe+3HQz7VP1peCsnPA20KEP4OLuQUx3q34JB8fOe2g/8w7K?=
 =?us-ascii?Q?vFhS9uaIDPLAm4vaCr6PA1UnksSeMtWoesRFpcFPH/BjDfEo6A9R/3TKwBlJ?=
 =?us-ascii?Q?fGH85MiZd3gZ0rePlJe+owMu13cR0xsU39MbDk5o2xlF8JX8JPuDxRn8Tnja?=
 =?us-ascii?Q?wcA1p35qYM2vwE0d8V/XPfUiyjkaurrlOhuWbS1cocWl+HGaJT3xN3Zlt5bY?=
 =?us-ascii?Q?VMEHkyuSugbiuDWlAB7ysp3+eeEMTQCLa8xzdepPPCgg9jWRoD+FUyBic7LH?=
 =?us-ascii?Q?4hp/B+sync7bWnK9wqHECuPjQLFGmmzWmjzhqpMt2o2Yv8tv8pbDwk09WS4n?=
 =?us-ascii?Q?TfbRD6WOPRGrPY/JLWNij+K9MisNajJt9pZP55STJ6L4160nuiPmcRpJDJv9?=
 =?us-ascii?Q?SlqmvWpoJytUMX4toyENGQ+/+1AJtgmYPCsUcQhAv36JiHyI/LwQHui+SzbN?=
 =?us-ascii?Q?looYWPjVUmf5hVhtVSYcT6bs1OqbY8cD1NF8bF6WkXXfN9dCgv5bAexef/6c?=
 =?us-ascii?Q?NnVvwewwgMrOuFOIiekGBwmq?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8d4833b5-f878-4f9f-ee19-08d994c0ebfb
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2966.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Oct 2021 18:30:56.4583
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: daniel.m.jordan@oracle.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3270
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10144 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 adultscore=0
 spamscore=0 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2110210092
X-Proofpoint-GUID: yt-dRxa_pW4psLl4_QrAk74B7rNuy5oW
X-Proofpoint-ORIG-GUID: yt-dRxa_pW4psLl4_QrAk74B7rNuy5oW
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

sk->sk_err appears to expect a positive value, a convention that ktls
doesn't always follow and that leads to memory corruption in other code.
For instance,

    [task1]
    tls_encrypt_done(..., err=<negative error from crypto request>)
      tls_err_abort(.., err)
        sk->sk_err = err;

    [task2]
    splice_from_pipe_feed
      ...
        tls_sw_do_sendpage
          if (sk->sk_err) {
            ret = -sk->sk_err;  // ret is positive

    splice_from_pipe_feed (continued)
      ret = actor(...)  // ret is still positive and interpreted as bytes
                        // written, resulting in underflow of buf->len and
                        // sd->len, leading to huge buf->offset and bogus
                        // addresses computed in later calls to actor()

Fix all tls_err_abort() callers to pass a negative error code
consistently and centralize the error-prone sign flip there, throwing in
a warning to catch future misuse.

Cc: stable@vger.kernel.org
Fixes: c46234ebb4d1e ("tls: RX path for ktls")
Reported-by: syzbot+b187b77c8474f9648fae@syzkaller.appspotmail.com
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
---

I could be wrong about sk->sk_err expecting a positive value, but at
least the sign of the error code is inconsistent.  Open to suggestions.

 include/net/tls.h | 7 +++++--
 net/tls/tls_sw.c  | 8 ++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index be4b3e1cac46..206505df2f1c 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -36,6 +36,7 @@
 
 #include <linux/types.h>
 #include <asm/byteorder.h>
+#include <linux/bug.h>
 #include <linux/crypto.h>
 #include <linux/socket.h>
 #include <linux/tcp.h>
@@ -468,7 +469,9 @@ static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
 
 static inline void tls_err_abort(struct sock *sk, int err)
 {
-	sk->sk_err = err;
+	WARN_ON_ONCE(err >= 0);
+	/* sk->sk_err should contain a positive error code. */
+	sk->sk_err = -err;
 	sk_error_report(sk);
 }
 
@@ -512,7 +515,7 @@ static inline void tls_advance_record_sn(struct sock *sk,
 					 struct cipher_context *ctx)
 {
 	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
-		tls_err_abort(sk, EBADMSG);
+		tls_err_abort(sk, -EBADMSG);
 
 	if (prot->version != TLS_1_3_VERSION &&
 	    prot->cipher_type != TLS_CIPHER_CHACHA20_POLY1305)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 4feb95e34b64..9705262749e6 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -419,7 +419,7 @@ int tls_tx_records(struct sock *sk, int flags)
 
 tx_err:
 	if (rc < 0 && rc != -EAGAIN)
-		tls_err_abort(sk, EBADMSG);
+		tls_err_abort(sk, -EBADMSG);
 
 	return rc;
 }
@@ -763,7 +763,7 @@ static int tls_push_record(struct sock *sk, int flags,
 			       msg_pl->sg.size + prot->tail_size, i);
 	if (rc < 0) {
 		if (rc != -EINPROGRESS) {
-			tls_err_abort(sk, EBADMSG);
+			tls_err_abort(sk, -EBADMSG);
 			if (split) {
 				tls_ctx->pending_open_record_frags = true;
 				tls_merge_open_record(sk, rec, tmp, orig_end);
@@ -1827,7 +1827,7 @@ int tls_sw_recvmsg(struct sock *sk,
 		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
 					 &chunk, &zc, async_capable);
 		if (err < 0 && err != -EINPROGRESS) {
-			tls_err_abort(sk, EBADMSG);
+			tls_err_abort(sk, -EBADMSG);
 			goto recv_end;
 		}
 
@@ -2007,7 +2007,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		}
 
 		if (err < 0) {
-			tls_err_abort(sk, EBADMSG);
+			tls_err_abort(sk, -EBADMSG);
 			goto splice_read_end;
 		}
 		ctx->decrypted = 1;
-- 
2.27.0