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,
