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
| ||
|
Date: Wed, 18 Jan 2017 14:15:26 -0800 From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> To: Paolo Bonzini <pbonzini@...hat.com> Cc: Dmitry Vyukov <dvyukov@...gle.com>, Steve Rutherford <srutherford@...gle.com>, syzkaller <syzkaller@...glegroups.com>, Radim Krčmář <rkrcmar@...hat.com>, KVM list <kvm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org> Subject: Re: kvm: use-after-free in process_srcu On Wed, Jan 18, 2017 at 09:53:19AM +0100, Paolo Bonzini wrote: > > > On 17/01/2017 21:34, Paul E. McKenney wrote: > > Do any of your callback functions invoke call_srcu()? (Hey, I have to ask!) > > No, we only use synchronize_srcu and synchronize_srcu_expedited, so our > only callback comes from there. OK, so the next question is whether your code makes sure that all of its synchronize_srcu() and synchronize_srcu_expedited() calls return before the call to cleanup_srcu_struct(). > >>>> From: Paolo Bonzini <pbonzini@...hat.com> > >>>> Subject: [PATCH] srcu: wait for all callbacks before deeming SRCU "cleaned up" > >>>> > >>>> Even though there are no concurrent readers, it is possible that the > >>>> work item is queued for delayed processing when cleanup_srcu_struct is > >>>> called. The work item needs to be flushed before returning, or a > >>>> use-after-free can ensue. > >>>> > >>>> Furthermore, because of SRCU's two-phase algorithm it may take up to > >>>> two executions of srcu_advance_batches before all callbacks are invoked. > >>>> This can happen if the first flush_delayed_work happens as follows > >>>> > >>>> srcu_read_lock > >>>> process_srcu > >>>> srcu_advance_batches > >>>> ... > >>>> if (!try_check_zero(sp, idx^1, trycount)) > >>>> // there is a reader > >>>> return; > >>>> srcu_invoke_callbacks > >>>> ... > >>>> srcu_read_unlock > >>>> cleanup_srcu_struct > >>>> flush_delayed_work > >>>> srcu_reschedule > >>>> queue_delayed_work > >>>> > >>>> Now flush_delayed_work returns but srcu_reschedule will *not* have cleared > >>>> sp->running to false. > > > > But srcu_reschedule() sets sp->running to false if there are no callbacks. > > And at that point, there had better be no callbacks. > > There must be no callbacks in batch_queue and in batch_check0, and of > course srcu_invoke_callbacks will have emptied batch_done as well. > > However, I'm not sure that batch_check1 is always empty after the first > flush_delayed_work *if there's no srcu_barrier* in the caller. > srcu_advance_batches's second call to try_check_zero could have failed, > and then srcu_reschedule will requeue the work item to advance > batch_check1 into batch_done. You should only need srcu_barrier() if there were calls to call_srcu(). Given that you only have synchronize_srcu() and synchronize_srcu_expedited(), you -don't- need srcu_barrier(). What you need instead is to make sure that all synchronize_srcu() and synchronize_srcu_expedited() have returned before the call to cleanup_srcu_struct(). > If this is incorrect, then one flush_delayed_work is enough. If it is > correct, the possible alternatives are: > > * srcu_barrier in the caller, flush_delayed_work+WARN_ON(sp->running) in > cleanup_srcu_struct. I strongly dislike this one---because we don't use > call_srcu at all, there should be no reason to use srcu_barrier in KVM > code. Plus I think all other users have the same issue. > > * srcu_barrier+flush_delayed_work+WARN_ON(sp->running) in > cleanup_srcu_struct > > * flush_delayed_work+flush_delayed_work+WARN_ON(sp->running) in > cleanup_srcu_struct > > * while(flush_delayed_work) in cleanup_srcu_struct > > * "while(sp->running) flush_delayed_work" in cleanup_srcu_struct My current thought is flush_delayed_work() followed by a warning if there are any callbacks still posted, and also as you say sp->running. Thoughts? Thanx, Paul
Powered by blists - more mailing lists