| 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: Wed, 06 May 2015 10:00:30 -0600
From: Toshi Kani <toshi.kani@...com>
To: Borislav Petkov <bp@...en8.de>
Cc: akpm@...ux-foundation.org, hpa@...or.com, tglx@...utronix.de,
mingo@...hat.com, linux-mm@...ck.org, x86@...nel.org,
linux-kernel@...r.kernel.org, dave.hansen@...el.com,
Elliott@...com, pebolle@...cali.nl
Subject: Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()
On Wed, 2015-05-06 at 15:41 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:40PM -0600, Toshi Kani wrote:
> > MTRRs contain fixed and variable entries. mtrr_type_lookup()
> > may repeatedly call __mtrr_type_lookup() to handle a request
> > that overlaps with variable entries. However,
> > __mtrr_type_lookup() also handles the fixed entries, which
> > do not have to be repeated. Therefore, this patch creates
> > separate functions, mtrr_type_lookup_fixed() and
> > mtrr_type_lookup_variable(), to handle the fixed and variable
> > ranges respectively.
> >
> > The patch also updates the function headers to clarify the
> > return values and output argument. It updates comments to
> > clarify that the repeating is necessary to handle overlaps
> > with the default type, since overlaps with multiple entries
> > alone can be handled without such repeating.
> >
> > There is no functional change in this patch.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@...com>
> > ---
> > arch/x86/kernel/cpu/mtrr/generic.c | 137 +++++++++++++++++++++++-------------
> > 1 file changed, 86 insertions(+), 51 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > index 8bd1298..3652e2b 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -102,55 +102,69 @@ static int check_type_overlap(u8 *prev, u8 *curr)
> > return 0;
> > }
> >
> > -/*
> > - * Error/Semi-error returns:
> > - * MTRR_TYPE_INVALID - when MTRR is not enabled
> > - * *repeat == 1 implies [start:end] spanned across MTRR range and type returned
> > - * corresponds only to [start:*partial_end].
> > - * Caller has to lookup again for [*partial_end:end].
> > +/**
> > + * mtrr_type_lookup_fixed - look up memory type in MTRR fixed entries
> > + *
> > + * MTRR fixed entries are divided into the following ways:
> > + * 0x00000 - 0x7FFFF : This range is divided into eight 64KB sub-ranges
> > + * 0x80000 - 0xBFFFF : This range is divided into sixteen 16KB sub-ranges
> > + * 0xC0000 - 0xFFFFF : This range is divided into sixty-four 4KB sub-ranges
>
> No need for those - simply a pointer to either the SDM or APM manuals'
> section suffices as they both describe it good.
Ingo asked me to describe this info here in his review...
> > + *
> > + * Return Values:
> > + * MTRR_TYPE_(type) - Matched memory type
> > + * MTRR_TYPE_INVALID - Unmatched or fixed entries are disabled
> > */
> > -static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> > +static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> > +{
> > + int idx;
> > +
> > + if (start >= 0x100000)
> > + return MTRR_TYPE_INVALID;
> > +
> > + if (!(mtrr_state.have_fixed) ||
> > + !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> > + return MTRR_TYPE_INVALID;
> > +
> > + if (start < 0x80000) { /* 0x0 - 0x7FFFF */
> > + idx = 0;
> > + idx += (start >> 16);
> > + return mtrr_state.fixed_ranges[idx];
> > +
> > + } else if (start < 0xC0000) { /* 0x80000 - 0xBFFFF */
> > + idx = 1 * 8;
> > + idx += ((start - 0x80000) >> 14);
> > + return mtrr_state.fixed_ranges[idx];
> > + }
> > +
> > + /* 0xC0000 - 0xFFFFF */
> > + idx = 3 * 8;
> > + idx += ((start - 0xC0000) >> 12);
> > + return mtrr_state.fixed_ranges[idx];
> > +}
> > +
> > +/**
> > + * mtrr_type_lookup_variable - look up memory type in MTRR variable entries
> > + *
> > + * Return Value:
> > + * MTRR_TYPE_(type) - Matched memory type or default memory type (unmatched)
> > + *
> > + * Output Argument:
> > + * repeat - Set to 1 when [start:end] spanned across MTRR range and type
> > + * returned corresponds only to [start:*partial_end]. Caller has
> > + * to lookup again for [*partial_end:end].
> > + */
> > +static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > + int *repeat)
> > {
> > int i;
> > u64 base, mask;
> > u8 prev_match, curr_match;
> >
> > *repeat = 0;
> > - if (!mtrr_state_set)
> > - return MTRR_TYPE_INVALID;
> > -
> > - if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
> > - return MTRR_TYPE_INVALID;
> >
> > /* Make end inclusive end, instead of exclusive */
> > end--;
> >
> > - /* Look in fixed ranges. Just return the type as per start */
> > - if ((start < 0x100000) &&
> > - (mtrr_state.have_fixed) &&
> > - (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
> > - int idx;
> > -
> > - if (start < 0x80000) {
> > - idx = 0;
> > - idx += (start >> 16);
> > - return mtrr_state.fixed_ranges[idx];
> > - } else if (start < 0xC0000) {
> > - idx = 1 * 8;
> > - idx += ((start - 0x80000) >> 14);
> > - return mtrr_state.fixed_ranges[idx];
> > - } else {
> > - idx = 3 * 8;
> > - idx += ((start - 0xC0000) >> 12);
> > - return mtrr_state.fixed_ranges[idx];
> > - }
> > - }
> > -
> > - /*
> > - * Look in variable ranges
> > - * Look of multiple ranges matching this address and pick type
> > - * as per MTRR precedence
> > - */
> > prev_match = MTRR_TYPE_INVALID;
> > for (i = 0; i < num_var_ranges; ++i) {
> > unsigned short start_state, end_state, inclusive;
> > @@ -179,7 +193,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> > * Return the type for first region and a pointer to
> > * the start of second region so that caller will
> > * lookup again on the second region.
> > - * Note: This way we handle multiple overlaps as well.
> > + * Note: This way we handle overlaps with multiple
> > + * entries and the default type properly.
> > */
> > if (start_state)
> > *partial_end = base + get_mtrr_size(mask);
> > @@ -208,21 +223,18 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> > return curr_match;
> > }
> >
> > - if (mtrr_tom2) {
> > - if (start >= (1ULL<<32) && (end < mtrr_tom2))
> > - return MTRR_TYPE_WRBACK;
> > - }
> > -
> > if (prev_match != MTRR_TYPE_INVALID)
> > return prev_match;
> >
> > return mtrr_state.def_type;
> > }
> >
> > -/*
> > - * Returns the effective MTRR type for the region
> > - * Error return:
> > - * MTRR_TYPE_INVALID - when MTRR is not enabled
> > +/**
> > + * mtrr_type_lookup - look up memory type in MTRR
> > + *
> > + * Return Values:
> > + * MTRR_TYPE_(type) - The effective MTRR type for the region
> > + * MTRR_TYPE_INVALID - MTRR is disabled
> > */
> > u8 mtrr_type_lookup(u64 start, u64 end)
> > {
> > @@ -230,22 +242,45 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> > int repeat;
> > u64 partial_end;
> >
> > - type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
> > + if (!mtrr_state_set)
> > + return MTRR_TYPE_INVALID;
> > +
> > + if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
> > + return MTRR_TYPE_INVALID;
> > +
> > + /*
> > + * Look up the fixed ranges first, which take priority over
> > + * the variable ranges.
> > + */
> > + type = mtrr_type_lookup_fixed(start, end);
> > + if (type != MTRR_TYPE_INVALID)
> > + return type;
>
> Huh, why are we not looking at start?
>
> I mean, fixed MTRRs cover the first 1MB so we can simply do:
>
> if ((start < 0x100000) &&
> (mtrr_state.have_fixed) &&
> (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> return mtrr_type_lookup_fixed(start, end);
mtrr_type_lookup_fixed() checks the above conditions at entry, and
returns immediately with TYPE_INVALID. I think it is safer to have such
checks in mtrr_type_lookup_fixed() in case there will be multiple
callers.
> and for all the other ranges we would do the variable lookup:
>
> type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
> ...
>
> ?
>
> Although I don't know what the code is supposed to do when a region
> starts in the fixed range and overlaps its end, i,e, something like
> that:
>
> [ start ... 0x100000 ... end ]
>
> The current code would return a fixed range index and that would be not
> really correct.
>
> OTOH, this has been like this forever so maybe we don't care...
Right, and there is more. As the original code had comment "Just return
the type as per start", which I noticed that I had accidentally removed,
the code only returns the type of the start address. The fixed ranges
have multiple entries with different types. Hence, a given range may
overlap with multiple fixed entries. I will restore the comment in the
function header to clarify this limitation.
Thanks,
-Toshi
--
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