| Message ID | 20230410014526.4035442-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] ipv4: Fix potential uninit variable access buf in __ip_make_skb() | expand |
Ziyang Xuan wrote: > Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in > __ip6_make_skb()"). icmphdr does not in skb linear region under the > scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will > trigger the uninit variable access bug. > > Use a local variable icmp_type to carry the correct value in different > scenarios. > > Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > v2: > - Use sk->sk_type not sk->sk_socket->type. This is reached through raw_sendmsg raw_probe_proto_opt ip_push_pending_frames ip_finish_skb __ip_make_skb At this point, the icmp header has already been copied into skb linear area. Is the isue that icmp_hdr/skb_transport_header is not set in this path? > --- > net/ipv4/ip_output.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index 4e4e308c3230..21507c9ce0fc 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -1570,9 +1570,15 @@ struct sk_buff *__ip_make_skb(struct sock *sk, > cork->dst = NULL; > skb_dst_set(skb, &rt->dst); > > - if (iph->protocol == IPPROTO_ICMP) > - icmp_out_count(net, ((struct icmphdr *) > - skb_transport_header(skb))->type); > + if (iph->protocol == IPPROTO_ICMP) { > + u8 icmp_type; > + > + if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) > + icmp_type = fl4->fl4_icmp_type; > + else > + icmp_type = icmp_hdr(skb)->type; > + icmp_out_count(net, icmp_type); > + } > > ip_cork_release(cork); > out: > -- > 2.25.1 >
> Ziyang Xuan wrote: >> Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in >> __ip6_make_skb()"). icmphdr does not in skb linear region under the >> scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will >> trigger the uninit variable access bug. >> >> Use a local variable icmp_type to carry the correct value in different >> scenarios. >> >> Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- >> v2: >> - Use sk->sk_type not sk->sk_socket->type. > > This is reached through > > raw_sendmsg > raw_probe_proto_opt > ip_push_pending_frames > ip_finish_skb > __ip_make_skb > > At this point, the icmp header has already been copied into skb linear > area. Is the isue that icmp_hdr/skb_transport_header is not set in > this path? > Hello Willem, raw_sendmsg ip_append_data(..., transhdrlen==0, ...) // !inet->hdrincl Parameter "transhdrlen" of ip_append_data() equals to 0 in raw_sendmsg() not sizeof(struct icmphdr) as in ping_v4_sendmsg(). William Xuan. >> --- >> net/ipv4/ip_output.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >> index 4e4e308c3230..21507c9ce0fc 100644 >> --- a/net/ipv4/ip_output.c >> +++ b/net/ipv4/ip_output.c >> @@ -1570,9 +1570,15 @@ struct sk_buff *__ip_make_skb(struct sock *sk, >> cork->dst = NULL; >> skb_dst_set(skb, &rt->dst); >> >> - if (iph->protocol == IPPROTO_ICMP) >> - icmp_out_count(net, ((struct icmphdr *) >> - skb_transport_header(skb))->type); >> + if (iph->protocol == IPPROTO_ICMP) { >> + u8 icmp_type; >> + >> + if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) >> + icmp_type = fl4->fl4_icmp_type; >> + else >> + icmp_type = icmp_hdr(skb)->type; >> + icmp_out_count(net, icmp_type); >> + } >> >> ip_cork_release(cork); >> out: >> -- >> 2.25.1 >> > > > . >
>> Ziyang Xuan wrote: >>> Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in >>> __ip6_make_skb()"). icmphdr does not in skb linear region under the >>> scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will >>> trigger the uninit variable access bug. >>> >>> Use a local variable icmp_type to carry the correct value in different >>> scenarios. >>> >>> Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") >>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >>> --- >>> v2: >>> - Use sk->sk_type not sk->sk_socket->type. >> >> This is reached through >> >> raw_sendmsg >> raw_probe_proto_opt >> ip_push_pending_frames >> ip_finish_skb >> __ip_make_skb >> >> At this point, the icmp header has already been copied into skb linear >> area. Is the isue that icmp_hdr/skb_transport_header is not set in >> this path? >> > Hello Willem, > > raw_sendmsg > ip_append_data(..., transhdrlen==0, ...) // !inet->hdrincl > > Parameter "transhdrlen" of ip_append_data() equals to 0 in raw_sendmsg() > not sizeof(struct icmphdr) as in ping_v4_sendmsg(). > > William Xuan. Hello Willem, Does my answer answer your doubts? William Xuan >>> --- >>> net/ipv4/ip_output.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >>> index 4e4e308c3230..21507c9ce0fc 100644 >>> --- a/net/ipv4/ip_output.c >>> +++ b/net/ipv4/ip_output.c >>> @@ -1570,9 +1570,15 @@ struct sk_buff *__ip_make_skb(struct sock *sk, >>> cork->dst = NULL; >>> skb_dst_set(skb, &rt->dst); >>> >>> - if (iph->protocol == IPPROTO_ICMP) >>> - icmp_out_count(net, ((struct icmphdr *) >>> - skb_transport_header(skb))->type); >>> + if (iph->protocol == IPPROTO_ICMP) { >>> + u8 icmp_type; >>> + >>> + if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) >>> + icmp_type = fl4->fl4_icmp_type; >>> + else >>> + icmp_type = icmp_hdr(skb)->type; >>> + icmp_out_count(net, icmp_type); >>> + } >>> >>> ip_cork_release(cork); >>> out: >>> -- >>> 2.25.1 >>> >> >> >> . >> > > . >
Ziyang Xuan (William) wrote: > >> Ziyang Xuan wrote: > >>> Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in > >>> __ip6_make_skb()"). icmphdr does not in skb linear region under the > >>> scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will > >>> trigger the uninit variable access bug. > >>> > >>> Use a local variable icmp_type to carry the correct value in different > >>> scenarios. > >>> > >>> Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") > >>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > >>> --- > >>> v2: > >>> - Use sk->sk_type not sk->sk_socket->type. > >> > >> This is reached through > >> > >> raw_sendmsg > >> raw_probe_proto_opt > >> ip_push_pending_frames > >> ip_finish_skb > >> __ip_make_skb > >> > >> At this point, the icmp header has already been copied into skb linear > >> area. Is the isue that icmp_hdr/skb_transport_header is not set in > >> this path? > >> > > Hello Willem, > > > > raw_sendmsg > > ip_append_data(..., transhdrlen==0, ...) // !inet->hdrincl > > > > Parameter "transhdrlen" of ip_append_data() equals to 0 in raw_sendmsg() > > not sizeof(struct icmphdr) as in ping_v4_sendmsg(). > > > > William Xuan. > Hello Willem, > > Does my answer answer your doubts? Thanks. Yes, that explains. Would you mind adding a comment to that effect. Sommething like /* for such sockets, transport hlen is zero and icmp_hdr incorrect */ if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) I missed your previous response, William. Sorry about that. > William Xuan > >>> --- > >>> net/ipv4/ip_output.c | 12 +++++++++--- > >>> 1 file changed, 9 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > >>> index 4e4e308c3230..21507c9ce0fc 100644 > >>> --- a/net/ipv4/ip_output.c > >>> +++ b/net/ipv4/ip_output.c > >>> @@ -1570,9 +1570,15 @@ struct sk_buff *__ip_make_skb(struct sock *sk, > >>> cork->dst = NULL; > >>> skb_dst_set(skb, &rt->dst); > >>> > >>> - if (iph->protocol == IPPROTO_ICMP) > >>> - icmp_out_count(net, ((struct icmphdr *) > >>> - skb_transport_header(skb))->type); > >>> + if (iph->protocol == IPPROTO_ICMP) { > >>> + u8 icmp_type; > >>> + > >>> + if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) > >>> + icmp_type = fl4->fl4_icmp_type; > >>> + else > >>> + icmp_type = icmp_hdr(skb)->type; > >>> + icmp_out_count(net, icmp_type); > >>> + } > >>> > >>> ip_cork_release(cork); > >>> out: > >>> -- > >>> 2.25.1 > >>> > >> > >> > >> . > >> > > > > . > >
> Ziyang Xuan (William) wrote: >>>> Ziyang Xuan wrote: >>>>> Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in >>>>> __ip6_make_skb()"). icmphdr does not in skb linear region under the >>>>> scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will >>>>> trigger the uninit variable access bug. >>>>> >>>>> Use a local variable icmp_type to carry the correct value in different >>>>> scenarios. >>>>> >>>>> Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") >>>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >>>>> --- >>>>> v2: >>>>> - Use sk->sk_type not sk->sk_socket->type. >>>> >>>> This is reached through >>>> >>>> raw_sendmsg >>>> raw_probe_proto_opt >>>> ip_push_pending_frames >>>> ip_finish_skb >>>> __ip_make_skb >>>> >>>> At this point, the icmp header has already been copied into skb linear >>>> area. Is the isue that icmp_hdr/skb_transport_header is not set in >>>> this path? >>>> >>> Hello Willem, >>> >>> raw_sendmsg >>> ip_append_data(..., transhdrlen==0, ...) // !inet->hdrincl >>> >>> Parameter "transhdrlen" of ip_append_data() equals to 0 in raw_sendmsg() >>> not sizeof(struct icmphdr) as in ping_v4_sendmsg(). >>> >>> William Xuan. >> Hello Willem, >> >> Does my answer answer your doubts? > > Thanks. Yes, that explains. Would you mind adding a comment to that > effect. Sommething like > > /* for such sockets, transport hlen is zero and icmp_hdr incorrect */ > if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) > That is fine. It will make other developers understand the logic here quickly. > I missed your previous response, William. Sorry about that. > >> William Xuan >>>>> --- >>>>> net/ipv4/ip_output.c | 12 +++++++++--- >>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >>>>> index 4e4e308c3230..21507c9ce0fc 100644 >>>>> --- a/net/ipv4/ip_output.c >>>>> +++ b/net/ipv4/ip_output.c >>>>> @@ -1570,9 +1570,15 @@ struct sk_buff *__ip_make_skb(struct sock *sk, >>>>> cork->dst = NULL; >>>>> skb_dst_set(skb, &rt->dst); >>>>> >>>>> - if (iph->protocol == IPPROTO_ICMP) >>>>> - icmp_out_count(net, ((struct icmphdr *) >>>>> - skb_transport_header(skb))->type); >>>>> + if (iph->protocol == IPPROTO_ICMP) { >>>>> + u8 icmp_type; >>>>> + >>>>> + if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) >>>>> + icmp_type = fl4->fl4_icmp_type; >>>>> + else >>>>> + icmp_type = icmp_hdr(skb)->type; >>>>> + icmp_out_count(net, icmp_type); >>>>> + } >>>>> >>>>> ip_cork_release(cork); >>>>> out: >>>>> -- >>>>> 2.25.1 >>>>> >>>> >>>> >>>> . >>>> >>> >>> . >>> > > > . >
Ziyang Xuan wrote: > Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in > __ip6_make_skb()"). icmphdr does not in skb linear region under the > scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will > trigger the uninit variable access bug. > > Use a local variable icmp_type to carry the correct value in different > scenarios. > > Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> Reviewed-by: Willem de Bruijn <willemb@google.com>
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 4e4e308c3230..21507c9ce0fc 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -1570,9 +1570,15 @@ struct sk_buff *__ip_make_skb(struct sock *sk, cork->dst = NULL; skb_dst_set(skb, &rt->dst); - if (iph->protocol == IPPROTO_ICMP) - icmp_out_count(net, ((struct icmphdr *) - skb_transport_header(skb))->type); + if (iph->protocol == IPPROTO_ICMP) { + u8 icmp_type; + + if (sk->sk_type == SOCK_RAW && !inet_sk(sk)->hdrincl) + icmp_type = fl4->fl4_icmp_type; + else + icmp_type = icmp_hdr(skb)->type; + icmp_out_count(net, icmp_type); + } ip_cork_release(cork); out:
Like commit ea30388baebc ("ipv6: Fix an uninit variable access bug in __ip6_make_skb()"). icmphdr does not in skb linear region under the scenario of SOCK_RAW socket. Access icmp_hdr(skb)->type directly will trigger the uninit variable access bug. Use a local variable icmp_type to carry the correct value in different scenarios. Fixes: 96793b482540 ("[IPV4]: Add ICMPMsgStats MIB (RFC 4293)") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- v2: - Use sk->sk_type not sk->sk_socket->type. --- net/ipv4/ip_output.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)