linux-kernel - Re: Bug with paravirt ops and livepatches

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: <alpine.LNX.2.00.1603291450090.26309@pobox.suse.cz>
Date:	Tue, 29 Mar 2016 15:01:22 +0200 (CEST)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Chris J Arges <chris.j.arges@...onical.com>
cc:	jeyu@...hat.com, jpoimboe@...hat.com, eugene.shatokhin@...alab.ru,
	live-patching@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	jikos@...nel.org, pmladek@...e.cz
Subject: Re: Bug with paravirt ops and livepatches

[ adding CCs ]

On Tue, 29 Mar 2016, Chris J Arges wrote:

> Paravirtualized ops and livepatching currently don't mix very well and can
> cause undefined behavor such as oops, invalid opcodes or corrupted stacks.
> The original discussion of this issue can be found here [1].
> 
> I've written an example livepatch module that reproduces the issue [2].
> In order to trigger the issue you must first insert the module then trigger
> the paravirt ops by starting a VM.
> 
> In the thread here [1] a couple of solutions have been proposed:

Hi,

oh no... so this is not only about paravirt ops but also about 
alternatives, jump labels and so on, isn't it?

> 1) Jessica proposed using the Arch-independent patchset ensure that livepatch
> finishes writing its relas before apply_paravirt() is called. However, this
> introduces a bit more arch-dependent code. It would be useful to see if other
> arches are affected by this as well.

I think this is the way to go. Provided we have Jessica's two patch sets 
applied (arch-independent and notifiers removal) there are two options. We 
either move a call to klp_coming_module() somewhere before 
module_finalize(), or we move the problematic parts of module_finalize() 
to the end of load_module() (on x86 it is probably module_finalize() as a 
whole). The former is almost impossible because of the dependencies 
(ftrace and such), the latter should be doable (with very careful check we 
won't break anything).

> 2) Eugene proposed skipping application of the rela if the instruction to be
> relocated has already been changed. This passes the initial example [2];
> however its unclear if/how this will break things.

Hm, I don't like this one. It really depends on that the paravirt 
instructions which are supposed to be patched do not contain the 
code which needs to be relocated. This can be true for now, but we have to 
think long-term... which leads me to... If the new instructions need to be 
relocated... this is indeed a problem, right? You'd need to fix 
kpatch-build somehow to generate appropriate dynrelas for the paravirt 
patched code. But, during the livepatch module generation one does not 
know if the code would be patched by alternatives. Crap :/

Miroslav

> It may be good to weigh in here and get more eyes on this.
> Thanks,
> --chris
> 
> [1]: https://github.com/dynup/kpatch/issues/580
> [2]: http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl/livepatch.c
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>