Re: [PATCH] libceph: fix client panic on MDS message with data but no data items

Re: [PATCH] libceph: fix client panic on MDS message with data but no data items

[Henry Martin]

Hi Alex,

Thanks for looking, but the skip you describe doesn't exist on the
MDS path.

mds_alloc_msg() (fs/ceph/mds_client.c:7384) leaves *skip = 0
unconditionally and never inspects hdr->data_len.  In v1,
read_partial_message() calls prepare_message_data() whenever the
on-wire data_len is non-zero (messenger_v1.c:1229); in v2,
__handle_control() routes any non-zero data_len to
prepare_read_data() (messenger_v2.c:1837).  Neither path compares
data_len to msg->data_length anywhere; I also found no such guard
in {mds,mon,osd}_client.c.

For an MDS message with data_len > 0 the first hit in
ceph_msg_data_cursor_init() is actually BUG_ON(length >
msg->data_length) with data_length == 0, one step before
BUG_ON(!msg->num_data_items); either way the client panics.

The check you have in mind is the OSD-side one, 98c4bfe9d89b2
("libceph: check reply num_data_items in setup_request_data()").
It validates reply items against the OSD request's preallocated
ops - MDS has no preallocated reply path at all, which is exactly
the sibling gap this patch closes.

Happy to move the rejection into mds_alloc_msg() (skip/reject
data_len > 0 there) if you prefer that over messenger-level
guards; I'd keep them since they also cover any future alloc_msg()
that allocates no items.

Regards,
Henry

[PATCH] libceph: fix client panic on MDS message with data but no data items

[Henry Martin]

mds_alloc_msg() allocates MDS->client messages with zero data items,
and nothing validates the on-wire data length against that: messenger
v1 calls prepare_message_data() whenever data_len is non-zero, and
messenger v2 unconditionally calls ceph_msg_data_cursor_init() from
prepare_read_data().  Both reach ceph_msg_data_cursor_init(), which
does BUG_ON(!msg->num_data_items) — a malicious or compromised MDS
can remotely panic any connected client (kernel denial of service) by
setting a non-zero data_len on any message type that carries no data
payload.

No legitimate MDS->client message carries data items (OSD messages
with data_len > 0 always attach items; MON/MDS control messages
carry none), so rejecting the combination is a pure protocol
violation check.  Turn the latent BUG_ON into a connection error
(-EPROTO, error_msg set) on both protocol versions.

This issue was discovered by Tencent CodeBuddy Security.

Cc: stable@vger.kernel.org
Fixes: 0d9c1ab3be4c ("libceph: preallocate message data items")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 net/ceph/messenger_v1.c | 7 ++++++-
 net/ceph/messenger_v2.c | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c
index c9e002d963190..d0978c9031a16 100644
--- a/net/ceph/messenger_v1.c
+++ b/net/ceph/messenger_v1.c
@@ -1226,8 +1226,13 @@ static int read_partial_message(struct ceph_connection *con)
 
 		/* prepare for data payload, if any */
 
-		if (data_len)
+		if (data_len) {
+			if (!con->in_msg->num_data_items) {
+				con->error_msg = "message carries data but msg has no data items";
+				return -EPROTO;
+			}
 			prepare_message_data(con->in_msg, data_len);
+		}
 	}
 
 	/* front */
diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c
index b323b61e70239..9ad43214bf984 100644
--- a/net/ceph/messenger_v2.c
+++ b/net/ceph/messenger_v2.c
@@ -1834,6 +1834,10 @@ static int prepare_read_data(struct ceph_connection *con)
 	struct bio_vec bv;
 
 	con->in_data_crc = -1;
+	if (!con->in_msg->num_data_items) {
+		con->error_msg = "frame carries data but msg has no data items";
+		return -EPROTO;
+	}
 	ceph_msg_data_cursor_init(&con->v2.in_cursor, con->in_msg,
 				  data_len(con->in_msg));
 
-- 
2.43.7