netdev - Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 13 May 2014 11:09:25 +0200
From:	Oliver Neukum <oneukum@...e.de>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Alexey Orishko <alexey.orishko@...il.com>,
	Enrico Mioso <mrkiko.rs@...il.com>,
	David Laight <David.Laight@...LAB.COM>
Subject: Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max
 update of setup

On Tue, 2014-05-13 at 10:49 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@...e.de> writes:
> 
> > On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote:

> >> +	/* clamp new_rx to sane values */
> >> +	min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> >> +	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> >
> > Are you sure this makes sense? min_t both times?
> 
> Yes, I am sure.  At least it made sense when I wrote it.  I am more in
> doubt now.

I actually suspected a copy n' paste error; thence the formulation.

> I guess you don't question the max calculation, but just so everyone
> else gets the idea: dwNtbInMaxSize is the buffer size suggested by the
> device. Some devices just specify an insanely large value (132kB has
> been observed). So we need to cap that to CDC_NCM_NTB_MAX_SIZE_RX, which
> is the absolutely largest buffer size we are prepared to support.

Good

> USB_CDC_NCM_NTB_MIN_IN_SIZE is the minimum acceptable buffer size
> according to the spec. dwNtbInMaxSize is not allowed to be smaller than
> this.  So if we assume that no device violates the spec, then the above
> should simple be
> 
> 	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
> 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> 
> which is the result for all spec conforming devices.
> 
> The reason I put that min_t() there instead was an attempt to deal with
> the (not unlikely) event that some buggy device set dwNtbInMaxSize lower
> than this required minimum value.  We then have the choices:
> 
>  a) fail to support the buggy device
>  b) attempt to set a larger buffer size than the device supports
>  c) accept the lower size

My preference would be b) > a) > c)
It seems to me that would should respect the spec and if the spec sets
a lower limit then we don't go lower.

> So I chose c) in an attempt to be as gentle as possible.  But I am open
> to go for a) instead if you think that is better. After all
> USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much
> more than the headers and a single full size ethernet frame.  And I see
> now that we fail to do further sanity checking after this.  What if
> dwNtbInMaxSize is 0? Or smaller than the necessary headers?

Exactly. Some fool may simply overlook setting it at all.

> Should I rewrite the above to do a) instead?  I.e.
> 
> 	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
> 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
>         if (min > max)
>            fail;
> 
> I don't think b) is a good idea.  It might work, but it might also fail
> in surprising ways making it hard to debug.

Users may prefer working devices to clean failures, but
I primarily care about conforming to spec. We just shouldn't
do such violations in a general case.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html