From: Dan Carpenter <error27@gmail.com>
To: Pavel Skripkin <paskripkin@gmail.com>
Cc: Phillip Potter <phil@philpotter.co.uk>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Deepak R Varma <drv@mailo.com>,
Charlie Sands <sandsch@northvilleschools.net>,
Mahak Gupta <mahak_g@cs.iitr.ac.in>,
Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>,
linux-staging@lists.linux.dev, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
Date: Thu, 23 Feb 2023 16:58:48 +0300 [thread overview]
Message-ID: <Y/dxGHm3kjjNzxqx@kadam> (raw)
In-Reply-To: <62e57016-c3e3-795c-afa2-8bbdb8071db6@gmail.com>
On Thu, Feb 23, 2023 at 02:00:48PM +0300, Pavel Skripkin wrote:
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index a7c67014dde0..f49e32c33372 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -538,7 +538,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
> > /*------------------------------------------------*/
> > struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
> > - if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> > + if (skb->len <= sizeof(*iph) + ETH_HLEN)
> > return -1;
>
>
> Thanks for the patch!
>
> I am wondering, if it make sense to use generic skb APIs which will do error
> handling for us?
>
> Like following (not even build-tested tho)
>
>
>
> With regards,
> Pavel Skripkin
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index a7c67014dde0..8f5f2ef26056 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -536,26 +536,29 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
> /*------------------------------------------------*/
> /* Handle IPV6 frame */
> /*------------------------------------------------*/
> - struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
> + u8 header *h = skb->data;
> + struct ipv6hdr *iph = skb_pull(skb, ETH_HLEN);
>
> - if (sizeof(*iph) >= (skb->len - ETH_HLEN))
> + if (!iph)
> return -1;
>
> switch (method) {
> case NAT25_CHECK:
> - if (skb->data[0] & 1)
> + if (h[0] & 1)
> return 0;
> return -1;
> case NAT25_INSERT:
> if (memcmp(&iph->saddr, "\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0", 16)) {
> __nat25_generate_ipv6_network_addr(addr, (unsigned int *)&iph->saddr);
> - __nat25_db_network_insert(priv, skb->data + ETH_ALEN, addr);
> + __nat25_db_network_insert(priv, (void *)iph, addr);
Doing it this way introduces a read overflow because there is no
guarantee that iph is large enough. You would still need a check for
if (skb->len < sizeof(*iph)). The existing check is <= instead of <,
but that was in the original code. I should have investigated why it is
<= and if it should be change to <. Sorry, this patch was not good.
A better way to do this would be to use skb_pull_data()... But it's
still buggy because it trusts be16_to_cpu(iph->payload_len). I need to
zoom out on this one. Is this NAT25 stuff even required? Do other
people do NAT stuff in their wifi drivers?
regards,
dan carpenter
diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index f49e32c33372..b84b2d4f23c3 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -536,27 +536,26 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
/*------------------------------------------------*/
/* Handle IPV6 frame */
/*------------------------------------------------*/
- struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + ETH_HLEN);
+ u8 *header = skb_pull_data(skb, ETH_HLEN);
+ struct ipv6hdr *iph = skb_pull_data(skb, sizeof(*iph));
- if (skb->len <= sizeof(*iph) + ETH_HLEN)
+ if (!iph)
return -1;
switch (method) {
case NAT25_CHECK:
- if (skb->data[0] & 1)
+ if (header[0] & 1)
return 0;
return -1;
case NAT25_INSERT:
if (memcmp(&iph->saddr, "\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0\x0", 16)) {
__nat25_generate_ipv6_network_addr(addr, (unsigned int *)&iph->saddr);
- __nat25_db_network_insert(priv, skb->data + ETH_ALEN, addr);
+ __nat25_db_network_insert(priv, iph, addr);
- if (iph->nexthdr == IPPROTO_ICMPV6 &&
- skb->len > (ETH_HLEN + sizeof(*iph) + 4)) {
- if (update_nd_link_layer_addr(skb->data + ETH_HLEN + sizeof(*iph),
- skb->len - ETH_HLEN - sizeof(*iph), GET_MY_HWADDR(priv))) {
- struct icmp6hdr *hdr = (struct icmp6hdr *)(skb->data + ETH_HLEN + sizeof(*iph));
- hdr->icmp6_cksum = 0;
+ if (iph->nexthdr == IPPROTO_ICMPV6 && skb->len > 4) {
+ if (update_nd_link_layer_addr(skb->data, skb->len,
+ GET_MY_HWADDR(priv))) {
+ struct icmp6hdr *hdr = (struct icmp6hdr *)skb->data;
hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
be16_to_cpu(iph->payload_len),
IPPROTO_ICMPV6,
next prev parent reply other threads:[~2023-02-23 13:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
2023-02-23 7:00 ` Philipp Hortmann
2023-02-23 11:00 ` Pavel Skripkin
2023-02-23 13:58 ` Dan Carpenter [this message]
2023-02-23 11:26 ` Dan Carpenter
2023-03-09 9:09 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y/dxGHm3kjjNzxqx@kadam \
--to=error27@gmail.com \
--cc=drv@mailo.com \
--cc=eng.alaamohamedsoliman.am@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=mahak_g@cs.iitr.ac.in \
--cc=paskripkin@gmail.com \
--cc=phil@philpotter.co.uk \
--cc=sandsch@northvilleschools.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox