Re: [PATCH] libceph: harden msgr2.1 frame segment length checks

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 7/12/23 20:07, Ilya Dryomov wrote:
> ceph_frame_desc::fd_lens is an int array.  decode_preamble() thus
> effectively casts u32 -> int but the checks for segment lengths are
> written as if on unsigned values.  While reading in HELLO or one of the
> AUTH frames (before authentication is completed), arithmetic in
> head_onwire_len() can get duped by negative ctrl_len and produce
> head_len which is less than CEPH_PREAMBLE_LEN but still positive.
> This would lead to a buffer overrun in prepare_read_control() as the
> preamble gets copied to the newly allocated buffer of size head_len.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)")
> Reported-by: Thelford Williams <thelford@xxxxxxxxxx>
> Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> ---
>   net/ceph/messenger_v2.c | 41 ++++++++++++++++++++++++++---------------
>   1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c
> index 1a888b86a494..1df1d29dee92 100644
> --- a/net/ceph/messenger_v2.c
> +++ b/net/ceph/messenger_v2.c
> @@ -390,6 +390,8 @@ static int head_onwire_len(int ctrl_len, bool secure)
>   	int head_len;
>   	int rem_len;
>   
> +	BUG_ON(ctrl_len < 0 || ctrl_len > CEPH_MSG_MAX_CONTROL_LEN);
> +
>   	if (secure) {
>   		head_len = CEPH_PREAMBLE_SECURE_LEN;
>   		if (ctrl_len > CEPH_PREAMBLE_INLINE_LEN) {
> @@ -408,6 +410,10 @@ static int head_onwire_len(int ctrl_len, bool secure)
>   static int __tail_onwire_len(int front_len, int middle_len, int data_len,
>   			     bool secure)
>   {
> +	BUG_ON(front_len < 0 || front_len > CEPH_MSG_MAX_FRONT_LEN ||
> +	       middle_len < 0 || middle_len > CEPH_MSG_MAX_MIDDLE_LEN ||
> +	       data_len < 0 || data_len > CEPH_MSG_MAX_DATA_LEN);
> +
>   	if (!front_len && !middle_len && !data_len)
>   		return 0;
>   
> @@ -520,29 +526,34 @@ static int decode_preamble(void *p, struct ceph_frame_desc *desc)
>   		desc->fd_aligns[i] = ceph_decode_16(&p);
>   	}
>   
> -	/*
> -	 * This would fire for FRAME_TAG_WAIT (it has one empty
> -	 * segment), but we should never get it as client.
> -	 */
> -	if (!desc->fd_lens[desc->fd_seg_cnt - 1]) {
> -		pr_err("last segment empty\n");
> +	if (desc->fd_lens[0] < 0 ||
> +	    desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) {
> +		pr_err("bad control segment length %d\n", desc->fd_lens[0]);
>   		return -EINVAL;
>   	}
> -
> -	if (desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) {
> -		pr_err("control segment too big %d\n", desc->fd_lens[0]);
> +	if (desc->fd_lens[1] < 0 ||
> +	    desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) {
> +		pr_err("bad front segment length %d\n", desc->fd_lens[1]);
>   		return -EINVAL;
>   	}
> -	if (desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) {
> -		pr_err("front segment too big %d\n", desc->fd_lens[1]);
> +	if (desc->fd_lens[2] < 0 ||
> +	    desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) {
> +		pr_err("bad middle segment length %d\n", desc->fd_lens[2]);
>   		return -EINVAL;
>   	}
> -	if (desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) {
> -		pr_err("middle segment too big %d\n", desc->fd_lens[2]);
> +	if (desc->fd_lens[3] < 0 ||
> +	    desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) {
> +		pr_err("bad data segment length %d\n", desc->fd_lens[3]);
>   		return -EINVAL;
>   	}
> -	if (desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) {
> -		pr_err("data segment too big %d\n", desc->fd_lens[3]);
> +
> +	/*
> +	 * This would fire for FRAME_TAG_WAIT (it has one empty
> +	 * segment), but we should never get it as client.
> +	 */
> +	if (!desc->fd_lens[desc->fd_seg_cnt - 1]) {
> +		pr_err("last segment empty, segment count %d\n",
> +		       desc->fd_seg_cnt);
>   		return -EINVAL;
>   	}
>   

LGTM.

Reviewed-by: Xiubo Li <xiubli@xxxxxxxxxx>