Re: [PATCH v2] libceph: reject monitor replies with oversized data segment

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Fri, 2026-05-01 at 23:37 +0400, Dhiraj Mishra wrote:
> Monitor messages can be allocated from preallocated reply messages or
> with ceph_msg_new(), both of which may provide only front-buffer storage
> and no data items.  The messenger receive path copies the wire header
> into the selected ceph_msg and later uses hdr.data_len to decide whether
> to initialize a data cursor.
> 
> If a malicious or compromised monitor advertises a non-zero data segment
> for one of these front-only replies, the receive path can call
> ceph_msg_data_cursor_init() with length greater than msg->data_length and
> hit its BUG_ON() checks, crashing the client kernel.
> 
> I verified the issue against v7.1-rc1-123-ge75a43c7cec4.  The msgr2
> trigger path is present since commit cd1a677cad99 ("libceph, ceph:
> implement msgr2.1 protocol (crc and secure modes)"), which is contained
> in v5.11-rc1 and later.  The monitor allocator pattern is older, but I
> have not tested older msgr1-only kernels.
> 
> A concrete trigger is a monitor connection over msgr2 after
> CEPH_CON_S_OPEN where a FRAME_TAG_MESSAGE contains a monitor reply type
> handled by mon_alloc_msg(), a valid front_len for that message type and
> data_len = 1.  CEPH_MSG_MON_MAP is one such example: the message is
> allocated with ceph_msg_new(), leaving msg->data_length and
> msg->num_data_items as zero.
> 
> Reject monitor replies whose wire data segment is larger than the data
> backing allocated for the selected ceph_msg, mirroring the existing OSD
> reply hardening.
> 
> Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)")
> Signed-off-by: Dhiraj Mishra <mishra.dhiraj95@gmail.com>
> ---
> v2:
>   - Resend as an inline plain-text patch.
>   - Add full email address to the From and Signed-off-by identities.
>   - Add ceph-devel and LKML to the recipient list when sending.
> 
>  net/ceph/mon_client.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
> index d2cdc8ee3155..7f2293122bba 100644
> --- a/net/ceph/mon_client.c
> +++ b/net/ceph/mon_client.c
> @@ -712,6 +712,7 @@ static struct ceph_msg *get_generic_reply(struct ceph_connection *con,
>  	struct ceph_mon_client *monc = con->private;
>  	struct ceph_mon_generic_request *req;
>  	u64 tid = le64_to_cpu(hdr->tid);
> +	u32 data_len = le32_to_cpu(hdr->data_len);
>  	struct ceph_msg *m;
>  
>  	mutex_lock(&monc->mutex);
> @@ -720,6 +721,16 @@ static struct ceph_msg *get_generic_reply(struct ceph_connection *con,
>  		dout("get_generic_reply %lld dne\n", tid);
>  		*skip = 1;
>  		m = NULL;
> +	} else if (!req->reply) {

The req->reply is set unconditionally when the generic request is allocated and
submitted; it is never NULL when req is found in the tree. It looks like a dead
code.

> +		pr_warn("%s tid %llu missing reply buffer, skipping\n",
> +			__func__, tid);
> +		*skip = 1;
> +		m = NULL;
> +	} else if (data_len > req->reply->data_length) {
> +		pr_warn("%s tid %llu data %u > preallocated %zu, skipping\n",
> +			__func__, tid, data_len, req->reply->data_length);

As far as I can see, other pr_err() calls in this file never show function name.
So, it's not consistent with common style of the module.

A malicious monitor can send a flood of oversized frames at line rate. Probably,
we need to consider pr_warn_ratelimited().

> +		*skip = 1;
> +		m = NULL;
>  	} else {
>  		dout("get_generic_reply %lld got %p\n", tid, req->reply);
>  		*skip = 0;
> @@ -1499,6 +1510,7 @@ static struct ceph_msg *mon_alloc_msg(struct ceph_connection *con,
>  	struct ceph_mon_client *monc = con->private;
>  	int type = le16_to_cpu(hdr->type);
>  	int front_len = le32_to_cpu(hdr->front_len);
> +	u32 data_len = le32_to_cpu(hdr->data_len);
>  	struct ceph_msg *m = NULL;
>  
>  	*skip = 0;
> @@ -1544,6 +1556,15 @@ static struct ceph_msg *mon_alloc_msg(struct ceph_connection *con,
>  		ceph_msg_put(m);
>  		m = ceph_msg_new(type, front_len, GFP_NOFS, false);
>  	}

I would like to see an empty line between these code blocks.

> +	if (m && data_len > m->data_length) {
> +		pr_warn("%s data %u > prealloc %zu (%u#%llu), skipping\n",
> +			__func__, data_len, m->data_length,
> +			(unsigned int)con->peer_name.type,
> +			le64_to_cpu(con->peer_name.num));
> +		ceph_msg_put(m);
> +		m = NULL;
> +		*skip = 1;
> +	}

I don't quite follow to the purpose of this code block. As far as I can see, we
have:

	case CEPH_MSG_STATFS_REPLY:
	case CEPH_MSG_MON_COMMAND_ACK:
		return get_generic_reply(con, hdr, skip);

What is the purpose of this?

Thanks,
Slava.

>  
>  	return m;
>  }