linux-kernel - Re: [PATCH v5] livepatch: fix race between fork and KLP transition

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:   Thu, 1 Sep 2022 15:45:50 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Rik van Riel <riel@...riel.com>
Cc:     Josh Poimboeuf <jpoimboe@...nel.org>, 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>
Subject: Re: [PATCH v5]  livepatch: fix race between fork and KLP transition

On Mon 2022-08-08 15:00:19, Rik van Riel wrote:
> The KLP transition code depends on the TIF_PATCH_PENDING and
> the task->patch_state to stay in sync. On a normal (forward)
> transition, TIF_PATCH_PENDING will be set on every task in
> the system, while on a reverse transition (after a failed
> forward one) first TIF_PATCH_PENDING will be cleared from
> every task, followed by it being set on tasks that need to
> be transitioned back to the original code.
> 
> However, the fork code copies over the TIF_PATCH_PENDING flag
> from the parent to the child early on, in dup_task_struct and
> 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;
>         }
> 
> Set, or clear, the TIF_PATCH_PENDING flag in the child task
> 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.
> 
> 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, and close the race for both normal and reverse transitions.
> 
> Signed-off-by: Rik van Riel <riel@...riel.com>
> Reported-by: Breno Leitao <leitao@...ian.org>
> Reviewed-by: Petr Mladek <pmladek@...e.com>
> Acked-by: Josh Poimboeuf <jpoimboe@...nel.org>
> Fixes: d83a7cb375ee ("livepatch: change to a per-task consistency model")
> Cc: stable@...nel.org

The patch has been pushed to livepatching/livepaching.git,
branch for-6.1/fixes.

Best Regards,
Petr