Hi Dan,
Dan Carpenter <error27@xxxxxxxxx> 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@xxxxxxxxx>
---
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
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),
