diff mbox series

USB: gadget: f_ncm: Fix NDP16 datagram validation

Message ID 20200920170158.1217068-1-bryan.odonoghue@linaro.org
State New
Headers show
Series USB: gadget: f_ncm: Fix NDP16 datagram validation | expand

Commit Message

Bryan O'Donoghue Sept. 20, 2020, 5:01 p.m. UTC
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(-)

Comments

Brooke Basile Sept. 20, 2020, 9:08 p.m. UTC | #1
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
Harald Seiler Sept. 24, 2020, 3:22 p.m. UTC | #2
On Sun, 2020-09-20 at 18:01 +0100, 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.


Same regression for me.  The last datagram of any NTB from the host is
ignored so most networking is broken.  With this patch, I'm able to
communicate from host to device again.

> 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>


Tested-by: Harald Seiler <hws@denx.de>

Reviewed-by: Harald Seiler <hws@denx.de>


> 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);

>  

> -- 

> 2.27.0

> 

-- 
Harald
Eugeniu Rosca Oct. 6, 2020, 6:29 p.m. UTC | #3
Hello Greg,
Hello stable maintainers,

On Sun, Sep 20, 2020 at 06:01:58PM +0100, 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(-)


This patch appears to be pushed to 4.19, 5.4 and 5.8 stable trees via:

https://lore.kernel.org/lkml/20201005142108.771915994@linuxfoundation.org/
https://lore.kernel.org/lkml/20201005142109.966570222@linuxfoundation.org/
https://lore.kernel.org/lkml/20201005142114.960807335@linuxfoundation.org/

Will you push this patch to v4.14.y, in order to fix v4.14.196 commit
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=b88ad6e714284b
("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()") ?

-- 
Best regards,
Eugeniu Rosca
Greg KH Oct. 6, 2020, 8:18 p.m. UTC | #4
On Tue, Oct 06, 2020 at 08:29:43PM +0200, Eugeniu Rosca wrote:
> Hello Greg,

> Hello stable maintainers,

> 

> On Sun, Sep 20, 2020 at 06:01:58PM +0100, 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(-)

> 

> This patch appears to be pushed to 4.19, 5.4 and 5.8 stable trees via:

> 

> https://lore.kernel.org/lkml/20201005142108.771915994@linuxfoundation.org/

> https://lore.kernel.org/lkml/20201005142109.966570222@linuxfoundation.org/

> https://lore.kernel.org/lkml/20201005142114.960807335@linuxfoundation.org/

> 

> Will you push this patch to v4.14.y, in order to fix v4.14.196 commit

> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=b88ad6e714284b

> ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()") ?


It also need to go to 4.9.y, and is already queued up in the stable
queues for those trees for the next round of kernels that are released
for them.

thanks,

greg k-h
Eugeniu Rosca Oct. 7, 2020, 10:24 a.m. UTC | #5
Hello again,

On Tue, Oct 06, 2020 at 10:18:25PM +0200, Greg KH wrote:
> On Tue, Oct 06, 2020 at 08:29:43PM +0200, Eugeniu Rosca wrote:

> > On Sun, Sep 20, 2020 at 06:01:58PM +0100, Bryan O'Donoghue wrote:

> > > commit 2b74b0a04d3e ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()")


[..]

> > This patch appears to be pushed to 4.19, 5.4 and 5.8 stable trees via:

> > 

> > https://lore.kernel.org/lkml/20201005142108.771915994@linuxfoundation.org/

> > https://lore.kernel.org/lkml/20201005142109.966570222@linuxfoundation.org/

> > https://lore.kernel.org/lkml/20201005142114.960807335@linuxfoundation.org/

> > 

> > Will you push this patch to v4.14.y, in order to fix v4.14.196 commit

> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=b88ad6e714284b

> > ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()") ?

> 

> It also need to go to 4.9.y, and is already queued up in the stable

> queues for those trees for the next round of kernels that are released

> for them.


Thank you!

-- 
Best regards,
Eugeniu Rosca
diff mbox series

Patch

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);