Re: [PATCH] USB: gadget: f_ncm: Fix NDP16 datagram validation

[Brooke Basile <brookebasile@gmail.com>]

On 9/20/20 1:01 PM, Bryan O'Donoghue wrote:
> commit 2b74b0a04d3e ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()")
> adds important bounds checking however it unfortunately also introduces  a
> bug with respect to section 3.3.1 of the NCM specification.
> 
> wDatagramIndex[1] : "Byte index, in little endian, of the second datagram
> described by this NDP16. If zero, then this marks the end of the sequence
> of datagrams in this NDP16."
> 
> wDatagramLength[1]: "Byte length, in little endian, of the second datagram
> described by this NDP16. If zero, then this marks the end of the sequence
> of datagrams in this NDP16."
> 
> wDatagramIndex[1] and wDatagramLength[1] respectively then may be zero but
> that does not mean we should throw away the data referenced by
> wDatagramIndex[0] and wDatagramLength[0] as is currently the case.
> 
> Breaking the loop on (index2 == 0 || dg_len2 == 0) should come at the end
> as was previously the case and checks for index2 and dg_len2 should be
> removed since zero is valid.
> 
> I'm not sure how much testing the above patch received but for me right now
> after enumeration ping doesn't work. Reverting the commit restores ping,
> scp, etc.
> 
> The extra validation associated with wDatagramIndex[0] and
> wDatagramLength[0] appears to be valid so, this change removes the incorrect
> restriction on wDatagramIndex[1] and wDatagramLength[1] restoring data
> processing between host and device.
> 
> Fixes: 2b74b0a04d3e ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()")
> Cc: Ilja Van Sprundel <ivansprundel@ioactive.com>
> Cc: Brooke Basile <brookebasile@gmail.com>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   drivers/usb/gadget/function/f_ncm.c | 30 ++---------------------------
>   1 file changed, 2 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index b4206b0dede5..1f638759a953 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -1189,7 +1189,6 @@ static int ncm_unwrap_ntb(struct gether *port,
>   	const struct ndp_parser_opts *opts = ncm->parser_opts;
>   	unsigned	crc_len = ncm->is_crc ? sizeof(uint32_t) : 0;
>   	int		dgram_counter;
> -	bool		ndp_after_header;
>   
>   	/* dwSignature */
>   	if (get_unaligned_le32(tmp) != opts->nth_sign) {
> @@ -1216,7 +1215,6 @@ static int ncm_unwrap_ntb(struct gether *port,
>   	}
>   
>   	ndp_index = get_ncm(&tmp, opts->ndp_index);
> -	ndp_after_header = false;
>   
>   	/* Run through all the NDP's in the NTB */
>   	do {
> @@ -1232,8 +1230,6 @@ static int ncm_unwrap_ntb(struct gether *port,
>   			     ndp_index);
>   			goto err;
>   		}
> -		if (ndp_index == opts->nth_size)
> -			ndp_after_header = true;
>   
>   		/*
>   		 * walk through NDP
> @@ -1312,37 +1308,13 @@ static int ncm_unwrap_ntb(struct gether *port,
>   			index2 = get_ncm(&tmp, opts->dgram_item_len);
>   			dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
>   
> -			if (index2 == 0 || dg_len2 == 0)
> -				break;
> -
>   			/* wDatagramIndex[1] */
> -			if (ndp_after_header) {
> -				if (index2 < opts->nth_size + opts->ndp_size) {
> -					INFO(port->func.config->cdev,
> -					     "Bad index: %#X\n", index2);
> -					goto err;
> -				}
> -			} else {
> -				if (index2 < opts->nth_size + opts->dpe_size) {
> -					INFO(port->func.config->cdev,
> -					     "Bad index: %#X\n", index2);
> -					goto err;
> -				}
> -			}
>   			if (index2 > block_len - opts->dpe_size) {
>   				INFO(port->func.config->cdev,
>   				     "Bad index: %#X\n", index2);
>   				goto err;
>   			}
>   
> -			/* wDatagramLength[1] */
> -			if ((dg_len2 < 14 + crc_len) ||
> -					(dg_len2 > frame_max)) {
> -				INFO(port->func.config->cdev,
> -				     "Bad dgram length: %#X\n", dg_len);
> -				goto err;
> -			}
> -
>   			/*
>   			 * Copy the data into a new skb.
>   			 * This ensures the truesize is correct
> @@ -1359,6 +1331,8 @@ static int ncm_unwrap_ntb(struct gether *port,
>   			ndp_len -= 2 * (opts->dgram_item_len * 2);
>   
>   			dgram_counter++;
> +			if (index2 == 0 || dg_len2 == 0)
> +				break;
>   		} while (ndp_len > 2 * (opts->dgram_item_len * 2));
>   	} while (ndp_index);
>   
> 
Bryan,

Ah, I see my mistake.  I did test this, but I must have missed some test 
cases as I didn't encounter this error.
Thanks a lot for testing and fixing the issue, and for the thorough 
explanation.

Best,
Brooke Basile