| 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: Tue, 1 Feb 2011 15:29:23 -0500
From: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To: Stefano Stabellini <stefano.stabellini@...citrix.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Xen-devel@...ts.xensource.com" <Xen-devel@...ts.xensource.com>,
"konrad@...nel.org" <konrad@...nel.org>,
"jeremy@...p.org" <jeremy@...p.org>,
"hpa@...or.com" <hpa@...or.com>,
Ian Campbell <Ian.Campbell@...citrix.com>
Subject: Re: [PATCH 11/11] xen/m2p: Check whether the MFN has
IDENTITY_FRAME bit set..
On Tue, Feb 01, 2011 at 05:52:29PM +0000, Stefano Stabellini wrote:
> On Mon, 31 Jan 2011, Konrad Rzeszutek Wilk wrote:
> > From: Stefano Stabellini <stefano.stabellini@...citrix.com>
> >
> > If we have the IDENTITY_FRAME bit set from the P2M, there
> > is no point in looking up MFN in the M2P override. This is
> > b/c the MFN is a physical MFN.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> > ---
> > arch/x86/include/asm/xen/page.h | 8 +++++++-
> > 1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> > index ed46ec2..e6f7f37 100644
> > --- a/arch/x86/include/asm/xen/page.h
> > +++ b/arch/x86/include/asm/xen/page.h
> > @@ -81,6 +81,7 @@ static inline int phys_to_machine_mapping_valid(unsigned long pfn)
> > static inline unsigned long mfn_to_pfn(unsigned long mfn)
> > {
> > unsigned long pfn;
> > + unsigned long p2m_mfn;
> >
> > if (xen_feature(XENFEAT_auto_translated_physmap))
> > return mfn;
> > @@ -102,7 +103,12 @@ try_override:
> > * doesn't map back to the mfn), then check the local override
> > * table to see if there's a better pfn to use.
> > */
> > - if (get_phys_to_machine(pfn) != mfn)
> > + p2m_mfn = get_phys_to_machine(pfn);
> > +
> > + if (p2m_mfn == IDENTITY_FRAME(mfn))
> > + return pfn;
> > +
> > + if (p2m_mfn != mfn)
> > pfn = m2p_find_override_pfn(mfn, pfn);
> >
> > return pfn;
>
>
> I have been thinking some more about it and now I have few questions:
>
> 1) is it possible for a single domain to have a valid mfn with the same
> number as an identity mfn (apart from the IDENTITY_FRAME bit)?
Yes.
>
> 2) is it true that mfn_to_pfn should never be called passing an identity
> mfn (because we set _PAGE_IOMAP)?
Yes. And currently the code checks for _PAGE_IOMAP and bypasses the
M2P.
>
> 3) what is the value returned by m2p(identity_mfn)? Is it a correct pfn
> or may be something like 0xfffff or 0xeeeee?
0xFFFFF... or 0x5555555..
>
>
> These are my guessed answers:
>
> 1) yes, it is possible.
> For example mfn=0xc0100 might be a valid ram mfn present in the mfn_list
> of a domU and also be present as 1:1 mfn of the 3G-4G region.
If we consider it valid, then it would be in the E820 as an E820_RAM
type. The xen_setup_identity code would skip over that region and not
mark is as IDENTITY.
Keep in mind the code skips over small/big E820_RAM regions even if
those regions have reserved E820 regions on both sides.
> For this reason I think we should look in m2p_override first and check
> for possible identity mapping later.
> We might want to avoid these situations but the only way I can see to do
> it would be to make sure that the 1:1 regions are always subset of
> the host reserved regions, even for domUs.
Right, and they are...
>
> 2) yes indeed.
> One more reason to look in the m2p_override first.
Not sure I understand.
>
> 3) the returned pfn might be 0xfffff or 0xeeeee.
> We should use the mfn value directly as pfn value to check for possible
> identity mappings.
Aren't we doing that via 'get_phys_to_machine' ? It returns the value
and if it has the IDENTITY_FRAME_BIT it is an identity.
Or are you thinking of abolishing the IDENTITY_FRAME_BIT and check the
M2P in conjunction with the P2M to see if the mfn is a 1-1 mapping?
>
>
> The resulting patch looks like the following:
>
> ---
>
>
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index ed46ec2..7f9bae2 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -80,6 +80,7 @@ static inline int phys_to_machine_mapping_valid(unsigned long pfn)
>
> static inline unsigned long mfn_to_pfn(unsigned long mfn)
> {
> + int ret = 0;
> unsigned long pfn;
>
> if (xen_feature(XENFEAT_auto_translated_physmap))
> @@ -95,15 +96,21 @@ static inline unsigned long mfn_to_pfn(unsigned long mfn)
> * In such cases it doesn't matter what we return (we return garbage),
> * but we must handle the fault without crashing!
> */
> - __get_user(pfn, &machine_to_phys_mapping[mfn]);
> + ret = __get_user(pfn, &machine_to_phys_mapping[mfn]);
> try_override:
> /*
> * If this appears to be a foreign mfn (because the pfn
> * doesn't map back to the mfn), then check the local override
> * table to see if there's a better pfn to use.
> */
> - if (get_phys_to_machine(pfn) != mfn)
> - pfn = m2p_find_override_pfn(mfn, pfn);
> + if (ret < 0)
> + pfn = ~0;
> + else if (get_phys_to_machine(pfn) != mfn)
> + pfn = m2p_find_override_pfn(mfn, ~0);
> +
> + if (pfn == ~0 &&
You should also check for 0x55555... then.
> + get_phys_to_machine(mfn) == IDENTITY_FRAME(mfn))
> + pfn = mfn;
So for identity type mfns we end up calling 'get_phys_to_machine(mfn)' twice
I think?
Would it make sense to save the result of 'get_phys_to_machine(mfn)'
the first call?
>
> return pfn;
> }
--
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/
Powered by blists - more mailing lists