netdev - Re: [PATCH] icmp: Fixed bug in raw sockets causing incorrect ICMP SNMP counter values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 12 Oct 2015 00:14:46 +0100
From:	Ben Cox <ben@...jojo.co.uk>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Ben Cox <ben@...jojo.co.uk>, davem@...emloft.net,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	ben+patches@...jojo.co.uk, David L Stevens <dlstevens@...ibm.com>
Subject: Re: [PATCH] icmp: Fixed bug in raw sockets causing incorrect ICMP
 SNMP counter values

Nice!

That works in my head at least, Sorry about not seeing that fairly
glaring memory issue there.

Are you sure " skb->transport_header += iphlen; " won't have a knock
on affect when it's given to NF_HOOK ( as in, would a potential
userspace program get something it does not expect anymore ) ?

How does submission work at this point if the above is not a issue
(apologies if this is already in a FAQ somewhere I missed)?

On Mon, Oct 12, 2015 at 12:09 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Sun, 2015-10-11 at 15:44 -0700, Eric Dumazet wrote:
>> On Sun, 2015-10-11 at 15:43 -0700, Eric Dumazet wrote:
>>
>> > But your code reads 21th byte.
>>
>> BTW, nice catch !
>
> Maybe the following one.
>
> 1) We properly set transport header
> 2) We use icmp_hdr() helper.
>
> diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
> index 561cd4b8fc6e..ffe25cd1f0e0 100644
> --- a/net/ipv4/raw.c
> +++ b/net/ipv4/raw.c
> @@ -406,11 +406,11 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
>                         ip_select_ident(net, skb, NULL);
>
>                 iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);
> +               skb->transport_header += iphlen;
> +               if (iph->protocol == IPPROTO_ICMP &&
> +                   length >= iphlen + sizeof(struct icmphdr))
> +                       icmp_out_count(net, icmp_hdr(skb)->type);
>         }
> -       if (iph->protocol == IPPROTO_ICMP)
> -               icmp_out_count(net, ((struct icmphdr *)
> -                       skb_transport_header(skb))->type);
> -
>         err = NF_HOOK(NFPROTO_IPV4, NF_INET_LOCAL_OUT, sk, skb,
>                       NULL, rt->dst.dev, dst_output_sk);
>         if (err > 0)
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html