netdev - Re: [PATCH net] tun: Fix NULL pointer dereference in XDP redirect

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] [day] [month] [year] [list]

Date:   Mon, 28 May 2018 13:28:33 +0900
From:   Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Toshiaki Makita <toshiaki.makita1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "Michael S. Tsirkin" <mst@...hat.com>
Subject: Re: [PATCH net] tun: Fix NULL pointer dereference in XDP redirect

On 2018/05/28 11:24, Jason Wang wrote:
> On 2018年05月25日 21:43, Toshiaki Makita wrote:
> 
> [...]
> 
>>>> @@ -1917,16 +1923,22 @@ static ssize_t tun_get_user(struct
>>>> tun_struct *tun, struct tun_file *tfile,
>>>>           struct bpf_prog *xdp_prog;
>>>>           int ret;
>>>> +        local_bh_disable();
>>>> +        preempt_disable();
>>>>           rcu_read_lock();
>>>>           xdp_prog = rcu_dereference(tun->xdp_prog);
>>>>           if (xdp_prog) {
>>>>               ret = do_xdp_generic(xdp_prog, skb);
>>>>               if (ret != XDP_PASS) {
>>>>                   rcu_read_unlock();
>>>> +                preempt_enable();
>>>> +                local_bh_enable();
>>>>                   return total_len;
>>>>               }
>>>>           }
>>>>           rcu_read_unlock();
>>>> +        preempt_enable();
>>>> +        local_bh_enable();
>>>>       }
>>>>       rcu_read_lock();
>>>
>>> Good catch, thanks.
>>>
>>> But I think we can just replace preempt_disable()/enable() with
>>> local_bh_disable()/local_bh_enable() ?
>>
>> I actually thought the same, but noticed this patch.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9ea4c380066fbe
>>
>>
>> It looks like they do not think local_bh_disable() implies
>> preempt_disable(). But I'm not sure why..
>>
>> Toshiaki Makita 
> 
> I see, there're probably have some subtle differences and implications
> for e.g scheduler or others.
> 
> What we what here is to make sure the process is not moved to another
> CPU and bh is enabled. By checking preemptible() function, preemption
> should be disabled after local_bh_disable(). So I think we're safe here.

OK. I checked retint_kernel which IIUC is the entry point of preemption
process on x86, and confirmed it just checks if __preempt_count is zero.

I haven't checked other archs but I was probably worried too much.
Will send v2.

Thanks,
Toshiaki Makita