netdev - Re: ipv6: handle -EFAULT from skb_copy

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 21 Dec 2016 14:04:31 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     hannes@...essinduktion.org
Cc:     xiyou.wangcong@...il.com, davej@...emonkey.org.uk,
        netdev@...r.kernel.org
Subject: Re: ipv6: handle -EFAULT from skb_copy_bits

From: Hannes Frederic Sowa <hannes@...essinduktion.org>
Date: Wed, 21 Dec 2016 13:41:13 +0100

> On Wed, 2016-12-21 at 13:27 +0100, Hannes Frederic Sowa wrote: 
>> @@ -555,8 +566,8 @@ static int rawv6_push_pending_frames(struct sock *sk, struct flowi6 *fl6,
>>                 goto out;
>>  
>>         offset = rp->offset;
>> -       total_len = inet_sk(sk)->cork.base.length;
>> -       if (offset >= total_len - 1) {
>> +       transport_len = raw6_corked_transport_len(sk);
>> +       if (offset >= transport_len - 1) {
>>                 err = -EINVAL;
>>                 ip6_flush_pending_frames(sk);
>>                 goto out;
>> @@ -598,7 +609,7 @@ static int rawv6_push_pending_frames(struct sock *sk, struct flowi6 *fl6,
>>                 tmp_csum = csum_sub(tmp_csum, csum_unfold(csum));
>>  
>>         csum = csum_ipv6_magic(&fl6->saddr, &fl6->daddr,
>> -                              total_len, fl6->flowi6_proto, tmp_csum);
>> +                              transport_len, fl6->flowi6_proto, tmp_csum);
>>  
>> 
> 
> Ops, here we need actually the total_len plus the opt->opt_nflen to
> always calculate the correct checksum.

It's a real shame we can't just use skb_transport_offset().  This value
has essentially been calculated for us already.

Also, if we iterate over multiple SKBs in the write queue, don't you have
to redo this calculation for every SKB we iterate over?

Furthermore, what if the user queued up some SKBs in the raw socket
with MSG_MORE, and then changed some of the options for a subsequent
sendmsg() call?

Given all of this, I think the best thing to do is validate the offset
after the queue walks, which is pretty much what Dave Jones's original
patch was doing.

Sigh... well, at least we now understand what's going on here.