linux-kernel - Re: [PATCH v4 1/2] XEN/X86: Improve semantic support for x86_init.mapping.pagetable

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]

Date:	Fri, 24 Aug 2012 15:00:14 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
cc:	Borislav Petkov <bp@...en8.de>,
	Attilio Rao <attilio.rao@...rix.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
	Stefano Stabellini <Stefano.Stabellini@...citrix.com>
Subject: Re: [PATCH v4 1/2] XEN/X86: Improve semantic support for
 x86_init.mapping.pagetable_reserve

On Fri, 24 Aug 2012, Konrad Rzeszutek Wilk wrote:
> His goal was to document the semantics of the call. We all want to clean
> up the mess of extra calls that don't make sense (remember the
> write_msr_safe one?) and the first step is get some of the calls
> documented so that we know if some of these calls can be moved around
> for refactoring. Attilio went then beyond that being enthuastic about
> this and wrote logic to deal with the description of the semantics.
> In part this would help the refactoring as it would catch runtime
> issues.

No. His logic to deal with the semantics started to imply wrong and
silly semantics in the first place. What's the point of making a
function deal with A != B, where A is required to be equal to B. We do
not add special cases for stuff which cannot happen neither on
baremetal nor on XEN. Period.

> That is at odds with what Peter would like to have fixed:
> (from
> http://lists.linux-foundation.org/pipermail/ksummit-2012-discuss/2012-June/000070.html)
> "
>    Hooks and notifiers are a form of "COME FROM" programming, and they
>    make it very hard to reason about the code.  The only way that that
>    can be reasonably mitigated is by having the exact semantics of a
>    hook or notifier -- the preconditions, postconditions, and other
>    invariants -- carefully documented.  Experience has shown that in
>    practice that happens somewhere between rarely and never.
> 
>    Hooks that terminate into hypercalls or otherwise are empty in the
>    "normal" flow are particularly problematic, as it is trivial for a
>    mainstream developer to break them.
> "

I'm not against documentation. I'm against wrong documentation, wrong
and silly semantics and pointless code which tries to deal with cases which
are just wrong to begin with.

I looked at the whole pgt_buf_* mess and it's amazingly stupid. We
could avoid all that dance and make all of that pgt_buf_* stuff static
and provide proper accessor functions and hand start, end, top to the
reserve function instead of fiddling with global variables all over
the place. That'd be a real cleanup and progress.

But we can't do that easily. And why? Because XEN is making magic
decisions based on those globals in mask_rw_pte().

	/*
	 * If the new pfn is within the range of the newly allocated
	 * kernel pagetable, and it isn't being mapped into an
	 * early_ioremap fixmap slot as a freshly allocated page, make sure
	 * it is RO.
	 */
	if (((!is_early_ioremap_ptep(ptep) &&
			pfn >= pgt_buf_start && pfn < pgt_buf_top)) ||
			(is_early_ioremap_ptep(ptep) && pfn != (pgt_buf_end - 1)))

This comment along with the implementation is really a master piece of
obfuscation. Let's see what this is doing. RO is enforced when:

This is not an early ioreamp AND

      pfn >= pgt_buf_start && pfn < pgt_buf_top

So why is this checking pgt_buf_top? The early stuff is installed
within pgt_buf_start and pgt_buf_end. Anything which is >=
pgt_buf_end at this point is completely wrong.

Now the second check is even more interesting:

If this is an early ioremap AND

      pfn != (pgt_buf_end -1 )

then it's forced RO as well.

So this checks whether the early ioremap is happening on the last
allocated pfn from the pgt_buf range.

OMG, really great design! And the comment above that if() obfuscation
is not really helping much.

If anything is missing a semantic documentation and analysis then
definitely code like this which is just a cobbled together steaming
pile of ....

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/