| Message ID | 20220210155455.4601-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] CDC-NCM: avoid overflow in sanity checking | expand |
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",
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
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
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
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
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
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
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",
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(-)