* [RFC] CDC-NCM: avoid overflow in sanity checking
@ 2022-02-10 15:54 Oliver Neukum
2022-02-10 16:38 ` Hans Petter Selasky
0 siblings, 1 reply; 8+ messages in thread
From: Oliver Neukum @ 2022-02-10 15:54 UTC (permalink / raw)
To: bjorn, linux-usb, netdev; +Cc: Oliver Neukum
A broken device may give an extreme offset like 0xFFF0
and a reasonable length for a fragment. In the sanity
check as formulated now, this will create an integer
overflow, defeating the sanity check. It needs to be
rewritten as a subtraction and the variables should be
unsigned.
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/net/usb/cdc_ncm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e303b522efb5..f78fccbc4b93 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1715,10 +1715,10 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
{
struct sk_buff *skb;
struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
- int len;
+ unsigned int len;
int nframes;
int x;
- int offset;
+ unsigned int offset;
union {
struct usb_cdc_ncm_ndp16 *ndp16;
struct usb_cdc_ncm_ndp32 *ndp32;
@@ -1791,7 +1791,7 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
}
/* sanity checking */
- if (((offset + len) > skb_in->len) ||
+ if ((offset > skb_in->len - len) ||
(len > ctx->rx_max) || (len < ETH_HLEN)) {
netif_dbg(dev, rx_err, dev->net,
"invalid frame detected (ignored) offset[%u]=%u, length=%u, skb=%p\n",
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-10 15:54 [RFC] CDC-NCM: avoid overflow in sanity checking Oliver Neukum
@ 2022-02-10 16:38 ` Hans Petter Selasky
2022-02-10 17:27 ` Bjørn Mork
0 siblings, 1 reply; 8+ messages in thread
From: Hans Petter Selasky @ 2022-02-10 16:38 UTC (permalink / raw)
To: Oliver Neukum, bjorn, linux-usb, netdev
On 2/10/22 16:54, Oliver Neukum wrote:
> A broken device may give an extreme offset like 0xFFF0
> and a reasonable length for a fragment. In the sanity
> check as formulated now, this will create an integer
> overflow, defeating the sanity check. It needs to be
> rewritten as a subtraction and the variables should be
> unsigned.
>
Hi Oliver,
First of all I'd like to update:
Hans Petter Selasky <hans.petter.selasky@stericsson.com>
To:
Hans Petter Selasky <hselasky@freebsd.org>
Secondly,
"int" variables are 32-bit, so 0xFFF0 won't overflow.
The initial driver code written by me did only support 16-bit lengths
and offset. Then integer overflow is not possible.
It looks like somebody else introduced this integer overflow :-(
commit 0fa81b304a7973a499f844176ca031109487dd31
Author: Alexander Bersenev <bay@hackerdom.ru>
Date: Fri Mar 6 01:33:16 2020 +0500
cdc_ncm: Implement the 32-bit version of NCM Transfer Block
The NCM specification defines two formats of transfer blocks: with
16-bit
fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
NTB-16 is
implemented.
....
With NCM 32, both "len" and "offset" must be checked, because these are
now 32-bit and stored into regular "int".
The fix you propose is not fully correct!
--HPS
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
> drivers/net/usb/cdc_ncm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index e303b522efb5..f78fccbc4b93 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1715,10 +1715,10 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
> {
> struct sk_buff *skb;
> struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> - int len;
> + unsigned int len;
> int nframes;
> int x;
> - int offset;
> + unsigned int offset;
> union {
> struct usb_cdc_ncm_ndp16 *ndp16;
> struct usb_cdc_ncm_ndp32 *ndp32;
> @@ -1791,7 +1791,7 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
> }
>
> /* sanity checking */
> - if (((offset + len) > skb_in->len) ||
> + if ((offset > skb_in->len - len) ||
> (len > ctx->rx_max) || (len < ETH_HLEN)) {
> netif_dbg(dev, rx_err, dev->net,
> "invalid frame detected (ignored) offset[%u]=%u, length=%u, skb=%p\n",
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-10 16:38 ` Hans Petter Selasky
@ 2022-02-10 17:27 ` Bjørn Mork
2022-02-10 22:50 ` Hans Petter Selasky
0 siblings, 1 reply; 8+ messages in thread
From: Bjørn Mork @ 2022-02-10 17:27 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: Oliver Neukum, linux-usb, netdev
Hans Petter Selasky <hps@selasky.org> writes:
> "int" variables are 32-bit, so 0xFFF0 won't overflow.
>
> The initial driver code written by me did only support 16-bit lengths
> and offset. Then integer overflow is not possible.
>
> It looks like somebody else introduced this integer overflow :-(
>
> commit 0fa81b304a7973a499f844176ca031109487dd31
> Author: Alexander Bersenev <bay@hackerdom.ru>
> Date: Fri Mar 6 01:33:16 2020 +0500
>
> cdc_ncm: Implement the 32-bit version of NCM Transfer Block
>
> The NCM specification defines two formats of transfer blocks: with
> 16-bit
> fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
> NTB-16 is
> implemented.
>
> ....
>
> With NCM 32, both "len" and "offset" must be checked, because these
> are now 32-bit and stored into regular "int".
>
> The fix you propose is not fully correct!
Yes, there is still an issue if len > skb_in->len since
(skb_in->len - len) then ends up as a very large unsigned int.
I must admit that I have some problems tweaking my mind around these
subtle unsigned overflow thingies. Which is why I suggest just
simplifying the whole thing with an additional test for the 32bit case
(which never will be used for any sane device):
} else {
offset = le32_to_cpu(dpe.dpe32->dwDatagramIndex);
len = le32_to_cpu(dpe.dpe32->dwDatagramLength);
if (offset < 0 || len < 0)
goto err_ndp;
}
And just keep the signed integers as-is. You cannot possible use all
bits of these anyway.
Yes, OK, that won't prevent offset + len from becoming negative, but
if will still work when compared to the unsigned skb_in->len.
Bjørn
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-10 17:27 ` Bjørn Mork
@ 2022-02-10 22:50 ` Hans Petter Selasky
2022-02-11 1:54 ` Alan Stern
0 siblings, 1 reply; 8+ messages in thread
From: Hans Petter Selasky @ 2022-02-10 22:50 UTC (permalink / raw)
To: Bjørn Mork; +Cc: Oliver Neukum, linux-usb, netdev
On 2/10/22 18:27, Bjørn Mork wrote:
> Hans Petter Selasky <hps@selasky.org> writes:
>
>> "int" variables are 32-bit, so 0xFFF0 won't overflow.
>>
>> The initial driver code written by me did only support 16-bit lengths
>> and offset. Then integer overflow is not possible.
>>
>> It looks like somebody else introduced this integer overflow :-(
>>
>> commit 0fa81b304a7973a499f844176ca031109487dd31
>> Author: Alexander Bersenev <bay@hackerdom.ru>
>> Date: Fri Mar 6 01:33:16 2020 +0500
>>
>> cdc_ncm: Implement the 32-bit version of NCM Transfer Block
>>
>> The NCM specification defines two formats of transfer blocks: with
>> 16-bit
>> fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
>> NTB-16 is
>> implemented.
>>
>> ....
>>
>> With NCM 32, both "len" and "offset" must be checked, because these
>> are now 32-bit and stored into regular "int".
>>
>> The fix you propose is not fully correct!
>
> Yes, there is still an issue if len > skb_in->len since
> (skb_in->len - len) then ends up as a very large unsigned int.
>
> I must admit that I have some problems tweaking my mind around these
> subtle unsigned overflow thingies. Which is why I suggest just
> simplifying the whole thing with an additional test for the 32bit case
> (which never will be used for any sane device):
>
> } else {
> offset = le32_to_cpu(dpe.dpe32->dwDatagramIndex);
> len = le32_to_cpu(dpe.dpe32->dwDatagramLength);
> if (offset < 0 || len < 0)
> goto err_ndp;
> }
Hi,
I think something like this would do the trick:
if (offset < 0 || offset > sbk_in->len ||
len < 0 || len > sbk_in->len)
>
> And just keep the signed integers as-is. You cannot possible use all
> bits of these anyway.
Right.
>
> Yes, OK, that won't prevent offset + len from becoming negative, but
> if will still work when compared to the unsigned skb_in->len.
>
--HPS
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-10 22:50 ` Hans Petter Selasky
@ 2022-02-11 1:54 ` Alan Stern
2022-02-11 7:17 ` Bjørn Mork
0 siblings, 1 reply; 8+ messages in thread
From: Alan Stern @ 2022-02-11 1:54 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: Bjørn Mork, Oliver Neukum, linux-usb, netdev
On Thu, Feb 10, 2022 at 11:50:20PM +0100, Hans Petter Selasky wrote:
> On 2/10/22 18:27, Bjørn Mork wrote:
> > Hans Petter Selasky <hps@selasky.org> writes:
> >
> > > "int" variables are 32-bit, so 0xFFF0 won't overflow.
> > >
> > > The initial driver code written by me did only support 16-bit lengths
> > > and offset. Then integer overflow is not possible.
> > >
> > > It looks like somebody else introduced this integer overflow :-(
> > >
> > > commit 0fa81b304a7973a499f844176ca031109487dd31
> > > Author: Alexander Bersenev <bay@hackerdom.ru>
> > > Date: Fri Mar 6 01:33:16 2020 +0500
> > >
> > > cdc_ncm: Implement the 32-bit version of NCM Transfer Block
> > >
> > > The NCM specification defines two formats of transfer blocks: with
> > > 16-bit
> > > fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
> > > NTB-16 is
> > > implemented.
> > >
> > > ....
> > >
> > > With NCM 32, both "len" and "offset" must be checked, because these
> > > are now 32-bit and stored into regular "int".
> > >
> > > The fix you propose is not fully correct!
> >
> > Yes, there is still an issue if len > skb_in->len since
> > (skb_in->len - len) then ends up as a very large unsigned int.
> >
> > I must admit that I have some problems tweaking my mind around these
> > subtle unsigned overflow thingies. Which is why I suggest just
> > simplifying the whole thing with an additional test for the 32bit case
> > (which never will be used for any sane device):
> >
> > } else {
> > offset = le32_to_cpu(dpe.dpe32->dwDatagramIndex);
> > len = le32_to_cpu(dpe.dpe32->dwDatagramLength);
> > if (offset < 0 || len < 0)
> > goto err_ndp;
> > }
>
> Hi,
>
> I think something like this would do the trick:
>
> if (offset < 0 || offset > sbk_in->len ||
> len < 0 || len > sbk_in->len)
Speaking as someone who is entirely unfamiliar with this code, a few
things seem clear.
First, since offset and len are initialized by converting 16- or 32-bit
unsigned values from little-endian to cpu-endian, they should be
unsigned themselves.
Second, once they are unsigned there is obviously no point in testing
whether they are < 0.
Third, if you want to make sure that skb_in's buffer contains the entire
interval from offset to offset + len, the proper tests are:
if (offset <= skb_in->len && len <= skb_in->len - offset) ...
The first test demonstrates that the start of the interval is in range
and the second test demonstrates that the end of the interval is in
range. Furthermore, success of the first test proves that the
computation in the second test can't overflow to a negative value.
IMO, working with unsigned values is simpler than working with
signed values. But it does require some discipline to ensure that
intermediate computations don't overflow or yield negative values.
Alan Stern
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-11 1:54 ` Alan Stern
@ 2022-02-11 7:17 ` Bjørn Mork
2022-02-14 19:30 ` Oliver Neukum
0 siblings, 1 reply; 8+ messages in thread
From: Bjørn Mork @ 2022-02-11 7:17 UTC (permalink / raw)
To: Alan Stern; +Cc: Hans Petter Selasky, Oliver Neukum, linux-usb, netdev
Alan Stern <stern@rowland.harvard.edu> writes:
> First, since offset and len are initialized by converting 16- or 32-bit
> unsigned values from little-endian to cpu-endian, they should be
> unsigned themselves.
>
> Second, once they are unsigned there is obviously no point in testing
> whether they are < 0.
>
> Third, if you want to make sure that skb_in's buffer contains the entire
> interval from offset to offset + len, the proper tests are:
>
> if (offset <= skb_in->len && len <= skb_in->len - offset) ...
>
> The first test demonstrates that the start of the interval is in range
> and the second test demonstrates that the end of the interval is in
> range. Furthermore, success of the first test proves that the
> computation in the second test can't overflow to a negative value.
Thanks. That detailed explanation makes perfect sense even to me.
Adding the additional offset <= skb_in->len test to Oliver's patch
is sufficient and the best solution.
Only is that the existing code wants the inverted result:
if (offset > skb_in->len || len > skb_in->len - offset) ...
with all values unsigned.
> IMO, working with unsigned values is simpler than working with
> signed values. But it does require some discipline to ensure that
> intermediate computations don't overflow or yield negative values.
And there you point out my problem: discipline :-)
Bjørn
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-11 7:17 ` Bjørn Mork
@ 2022-02-14 19:30 ` Oliver Neukum
2022-02-14 19:41 ` Bjørn Mork
0 siblings, 1 reply; 8+ messages in thread
From: Oliver Neukum @ 2022-02-14 19:30 UTC (permalink / raw)
To: Bjørn Mork, Alan Stern
Cc: Hans Petter Selasky, Oliver Neukum, linux-usb, netdev
On 11.02.22 08:17, Bjørn Mork wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
>
>
> Only is that the existing code wants the inverted result:
>
> if (offset > skb_in->len || len > skb_in->len - offset) ...
>
> with all values unsigned.
Its logic is
if (!sane(fragment))
continue;
process(fragment);
rather than
if (sane(fragment))
process(fragment);
A simple matter of inversion.
> And there you point out my problem: discipline :-)
>
Do we still agree that unsigned integers are the better option?
Regards
Oliver
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] CDC-NCM: avoid overflow in sanity checking
2022-02-14 19:30 ` Oliver Neukum
@ 2022-02-14 19:41 ` Bjørn Mork
0 siblings, 0 replies; 8+ messages in thread
From: Bjørn Mork @ 2022-02-14 19:41 UTC (permalink / raw)
To: Oliver Neukum; +Cc: Alan Stern, Hans Petter Selasky, linux-usb, netdev
Oliver Neukum <oneukum@suse.com> writes:
> Do we still agree that unsigned integers are the better option?
Yes. What Alan said made perfect sense. As always.
Bjørn
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-14 21:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-10 15:54 [RFC] CDC-NCM: avoid overflow in sanity checking Oliver Neukum
2022-02-10 16:38 ` Hans Petter Selasky
2022-02-10 17:27 ` Bjørn Mork
2022-02-10 22:50 ` Hans Petter Selasky
2022-02-11 1:54 ` Alan Stern
2022-02-11 7:17 ` Bjørn Mork
2022-02-14 19:30 ` Oliver Neukum
2022-02-14 19:41 ` Bjørn Mork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.