Re: [PATCH v4 02/10] smb: client: validate minimum PDU size before smb2_get_data_area_len()

[Paulo Alcantara <pc@manguebit.org>]

Frank Sorenson <sorenson@redhat.com> writes:

> __smb2_calc_size() calls smb2_get_data_area_len(), which reads
> command-specific struct fields to locate the data area. However,
> smb2_check_message() only validates StructureSize2, meaning a truncated
> response could cause smb2_get_data_area_len() to read out-of-bounds.
>
> Add smb2_min_pdu_len[] to track the size of the fixed response struct for
> each command with a data area, and reject PDUs shorter than this minimum
> before they are parsed.
>
> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
> ---
>  fs/smb/client/smb2misc.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
> index 9068175e57cd..22afc203a1ef 100644
> --- a/fs/smb/client/smb2misc.c
> +++ b/fs/smb/client/smb2misc.c
> @@ -85,6 +85,36 @@ static const __le16 smb2_rsp_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = {
>  	/* SMB2_OPLOCK_BREAK */ cpu_to_le16(24)
>  };
>  
> +/*
> + * Minimum received PDU size for commands whose fixed response struct is read
> + * by smb2_get_data_area_len() before the packet length is validated.  Must
> + * be non-zero for every command where has_smb2_data_area[] is true; zero
> + * otherwise (guard in smb2_check_message() is skipped).  Keep in sync with
> + * has_smb2_data_area[] above: adding a data area for a currently-zero command
> + * requires a matching sizeof() entry here.
> + */
> +static const size_t smb2_min_pdu_len[NUMBER_OF_SMB2_COMMANDS] = {
> +	/* SMB2_NEGOTIATE */       sizeof(struct smb2_negotiate_rsp),
> +	/* SMB2_SESSION_SETUP */   sizeof(struct smb2_sess_setup_rsp),
> +	/* SMB2_LOGOFF */          0,
> +	/* SMB2_TREE_CONNECT */    0,
> +	/* SMB2_TREE_DISCONNECT */ 0,
> +	/* SMB2_CREATE */          sizeof(struct smb2_create_rsp),
> +	/* SMB2_CLOSE */           0,
> +	/* SMB2_FLUSH */           0,
> +	/* SMB2_READ */            sizeof(struct smb2_read_rsp),
> +	/* SMB2_WRITE */           0,
> +	/* SMB2_LOCK */            0,
> +	/* SMB2_IOCTL */           sizeof(struct smb2_ioctl_rsp),
> +	/* SMB2_CANCEL */          0,
> +	/* SMB2_ECHO */            0,
> +	/* SMB2_QUERY_DIRECTORY */ sizeof(struct smb2_query_directory_rsp),
> +	/* SMB2_CHANGE_NOTIFY */   sizeof(struct smb2_change_notify_rsp),
> +	/* SMB2_QUERY_INFO */      sizeof(struct smb2_query_info_rsp),
> +	/* SMB2_SET_INFO */        0,
> +	/* SMB2_OPLOCK_BREAK */    0,
> +};

I don't understand why you're creating a new array.

What about to replace @has_smb2_data_area with the above array, then
have something like

        #define smb2_has_data_area(cmd) (smb2_min_pdu_len[cmd] != 0)

and in __smb2_calc_size()

        if (!smb2_has_data_area(le16_to_cpu(shdr->Command)))
                ....

Then we don't need to worry about keeping both arrays in sync.

> +
>  #define SMB311_NEGPROT_BASE_SIZE (sizeof(struct smb2_hdr) + sizeof(struct smb2_negotiate_rsp))
>  
>  static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len,
> @@ -233,6 +263,16 @@ smb2_check_message(char *buf, unsigned int pdu_len, unsigned int len,
>  		}
>  	}
>  
> +	if ((shdr->Status == 0 ||

nit: shdr->Status == STATUS_SUCCESS

> +	     shdr->Status == STATUS_MORE_PROCESSING_REQUIRED ||
> +	     pdu->StructureSize2 != SMB2_ERROR_STRUCTURE_SIZE2_LE) &&
> +	    smb2_min_pdu_len[command] &&
> +	    len < smb2_min_pdu_len[command]) {
> +		cifs_dbg(VFS, "SMB2 command %d response too short: %u < %zu\n",
> +			 command, len, smb2_min_pdu_len[command]);
> +		return 1;
> +	}
> +
>  	have_data = false;
>  	data_area_overlap = false;
>  	calc_len = __smb2_calc_size(buf, &have_data, &data_area_overlap);
> -- 
> 2.55.0