* [PATCH] staging: r8188eu: fix a potential integer underflow bug
@ 2023-02-22 13:59 Dan Carpenter
2023-02-23 7:00 ` Philipp Hortmann
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-02-22 13:59 UTC (permalink / raw)
To: Phillip Potter
Cc: Pavel Skripkin, Greg Kroah-Hartman, Deepak R Varma,
Charlie Sands, Mahak Gupta, Alaa Mohamed, linux-staging,
kernel-janitors
Here the code is testing to see if skb->len meets a minimum size
requirement. However if skb->len is very small then the ETH_HLEN
subtraction will result in a negative which is then type promoted
to an unsigned int and the condition will be true.
Generally, when you have an untrusted variable like skb->len, you
should move all the math to the other side of the comparison.
Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Compile tested only. This is basic algebra of moving parts of the
equation from one side to the other and I am surprisingly bad at
something that I was supposed to have learned in 9th grade.
drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
switch (method) {
--
2.39.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
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
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Philipp Hortmann @ 2023-02-23 7:00 UTC (permalink / raw)
To: Dan Carpenter, Phillip Potter
Cc: Pavel Skripkin, Greg Kroah-Hartman, Deepak R Varma,
Charlie Sands, Mahak Gupta, Alaa Mohamed, linux-staging,
kernel-janitors
On 2/22/23 14:59, Dan Carpenter wrote:
> Here the code is testing to see if skb->len meets a minimum size
> requirement. However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
>
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
>
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only. This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
>
> drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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;
>
> switch (method) {
Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
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
2023-02-23 11:26 ` Dan Carpenter
2023-03-09 9:09 ` Greg Kroah-Hartman
3 siblings, 1 reply; 6+ messages in thread
From: Pavel Skripkin @ 2023-02-23 11:00 UTC (permalink / raw)
To: Dan Carpenter, Phillip Potter
Cc: Greg Kroah-Hartman, Deepak R Varma, Charlie Sands, Mahak Gupta,
Alaa Mohamed, linux-staging, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
Hi Dan,
Dan Carpenter <error27@gmail.com> says:
> Here the code is testing to see if skb->len meets a minimum size
> requirement. However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
>
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
>
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only. This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
>
> drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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
[-- Attachment #2: ph1 --]
[-- Type: text/plain, Size: 1820 bytes --]
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);
+
+ if (iph->nexthdr == IPPROTO_ICMPV6) {
+ struct ipv6hdr *hdr = skb_pull(skb, sizeof(*iph));
+
+ if (!iph)
+ return 0;
- 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));
+ if (update_nd_link_layer_addr(hdr, skb_len(skb), GET_MY_HWADDR(priv))) {
hdr->icmp6_cksum = 0;
hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
be16_to_cpu(iph->payload_len),
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
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 11:26 ` Dan Carpenter
2023-03-09 9:09 ` Greg Kroah-Hartman
3 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-02-23 11:26 UTC (permalink / raw)
To: Phillip Potter
Cc: Pavel Skripkin, Greg Kroah-Hartman, Deepak R Varma,
Charlie Sands, Mahak Gupta, Alaa Mohamed, linux-staging,
kernel-janitors
On Wed, Feb 22, 2023 at 04:59:41PM +0300, Dan Carpenter wrote:
> Here the code is testing to see if skb->len meets a minimum size
> requirement. However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
>
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
>
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only. This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
>
> drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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;
NAK. On reviewing now, if this is a bug, then there is already a read
overflow a few lines earlier.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
2023-02-23 11:00 ` Pavel Skripkin
@ 2023-02-23 13:58 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-02-23 13:58 UTC (permalink / raw)
To: Pavel Skripkin
Cc: Phillip Potter, Greg Kroah-Hartman, Deepak R Varma,
Charlie Sands, Mahak Gupta, Alaa Mohamed, linux-staging,
kernel-janitors
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,
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: r8188eu: fix a potential integer underflow bug
2023-02-22 13:59 [PATCH] staging: r8188eu: fix a potential integer underflow bug Dan Carpenter
` (2 preceding siblings ...)
2023-02-23 11:26 ` Dan Carpenter
@ 2023-03-09 9:09 ` Greg Kroah-Hartman
3 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2023-03-09 9:09 UTC (permalink / raw)
To: Dan Carpenter
Cc: Phillip Potter, Pavel Skripkin, Deepak R Varma, Charlie Sands,
Mahak Gupta, Alaa Mohamed, linux-staging, kernel-janitors
On Wed, Feb 22, 2023 at 04:59:41PM +0300, Dan Carpenter wrote:
> Here the code is testing to see if skb->len meets a minimum size
> requirement. However if skb->len is very small then the ETH_HLEN
> subtraction will result in a negative which is then type promoted
> to an unsigned int and the condition will be true.
>
> Generally, when you have an untrusted variable like skb->len, you
> should move all the math to the other side of the comparison.
>
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only. This is basic algebra of moving parts of the
> equation from one side to the other and I am surprisingly bad at
> something that I was supposed to have learned in 9th grade.
>
> drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This driver is now deleted, so no need to worry about this anymore.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-09 9:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2023-02-23 11:26 ` Dan Carpenter
2023-03-09 9:09 ` Greg Kroah-Hartman
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.