linux-kernel - Re: [PATCH v3] livepatch: fix race between fork and klp_reverse

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]

Message-ID: <924f1cf4b93ff3e4ff217203bd88448dfa4c0f85.camel@surriel.com>
Date:   Tue, 26 Jul 2022 20:26:41 -0400
From:   Rik van Riel <riel@...riel.com>
To:     Josh Poimboeuf <jpoimboe@...nel.org>
Cc:     Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
        live-patching@...r.kernel.org, kernel-team@...com,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Breno Leitao <leitao@...ian.org>
Subject: Re: [PATCH v3] livepatch: fix race between fork and
 klp_reverse_transition

On Tue, 2022-07-26 at 17:10 -0700, Josh Poimboeuf wrote:
> On Mon, Jul 25, 2022 at 09:49:19AM -0400, Rik van Riel wrote:
> > When a KLP fails to apply, klp_reverse_transition will clear the
> > TIF_PATCH_PENDING flag on all tasks, except for newly created tasks
> > which are not on the task list yet.
> 
> This paragraph and $SUBJECT both talk about a reverse transition. 
> Isn't
> it also possible to race on a normal (forward) transition?

I don't know whether the race is also possible on a forward
transition.  If the parent task has transitioned, will
the child have, as well, by the time we reach the end of fork?

I suppose the only way the parent task can transition while
inside fork would be if none of the functions in its stack
need to be transitioned, and at that point the child process
would automatically be safe, too?

That would make copying the KLP transition state from parent to
child safe on a forward transition, too.

Am I overlooking anything?

However, we have only observed this warning on reverse transitions
for some reason.

> > Meanwhile, fork will copy over the TIF_PATCH_PENDING flag from the
> > parent to the child early on, in dup_task_struct ->
> > setup_thread_stack.
> > 
> > Much later, klp_copy_process will set child->patch_state to match
> > that of the parent.
> > 
> > However, the parent's patch_state may have been changed by KLP
> > loading
> > or unloading since it was initially copied over into the child.
> > 
> > This results in the KLP code occasionally hitting this warning in
> > klp_complete_transition:
> > 
> >         for_each_process_thread(g, task) {
> >                 WARN_ON_ONCE(test_tsk_thread_flag(task,
> > TIF_PATCH_PENDING));
> >                 task->patch_state = KLP_UNDEFINED;
> >         }
> > 
> > This patch will set, or clear, the TIF_PATCH_PENDING flag in the
> > child
> > process depending on whether or not it is needed at the time
> > klp_copy_process is called, at a point in copy_process where the
> > tasklist_lock is held exclusively, preventing races with the KLP
> > code.
> 
> Use imperative language, i.e. no "This patch".  See
> Documentation/process/submitting-patches.rst
> > 

Will do. I'll send a v4 tomorrow.

> > The KLP code does have a few places where the state is changed
> > without the tasklist_lock held, but those should not cause
> > problems because klp_update_patch_state(current) cannot be
> > called while the current task is in the middle of fork,
> > klp_check_and_switch_task() which is called under the pi_lock,
> > which prevents rescheduling, and manipulation of the patch
> > state of idle tasks, which do not fork.
> > 
> > This should prevent this warning from triggering again in the
> > future.
> > 
> 
> Fixes: d83a7cb375ee ("livepatch: change to a per-task consistency
> model")
> 
> > Signed-off-by: Rik van Riel <riel@...riel.com>
> > Reported-by: Breno Leitao <leitao@...ian.org>
> > Reviewed-by: Petr Mladek <pmladek@...e.com>
> 
> With the above minor things fixed:
> 
> Acked-by: Josh Poimboeuf <jpoimboe@...nel.org>
> 

-- 
All Rights Reversed.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)