On Tue, Mar 24, 2015 at 04:08:36PM -0600, Toshi Kani wrote:
> When an MTRR entry is inclusive to a requested range, i.e.
> the start and end of the request are not within the MTRR
> entry range but the range contains the MTRR entry entirely,
> __mtrr_type_lookup() ignores such a case because both
> start_state and end_state are set to zero.
>
> This bug can cause the following issues:
> 1) reserve_memtype() tracks an effective memory type in case
> a request type is WB (ex. /dev/mem blindly uses WB). Missing
> to track with its effective type causes a subsequent request
> to map the same range with the effective type to fail.
> 2) pud_set_huge() and pmd_set_huge() check if a requested range
> has any overlap with MTRRs. Missing to detect an overlap may
> cause a performance penalty or undefined behavior.
>
> This patch fixes the bug by adding a new flag, 'inclusive',
> to detect the inclusive case. This case is then handled in
> the same way as (!start_state && end_state). With this fix,
> __mtrr_type_lookup() handles the inclusive case properly.
>
> Signed-off-by: Toshi Kani <[email protected]>
> ---
> arch/x86/kernel/cpu/mtrr/generic.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 7d74f7b..a82e370 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -154,7 +154,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>
> prev_match = 0xFF;
> for (i = 0; i < num_var_ranges; ++i) {
> - unsigned short start_state, end_state;
> + unsigned short start_state, end_state, inclusive;
>
> if (!(mtrr_state.var_ranges[i].mask_lo & (1 << 11)))
> continue;
> @@ -166,15 +166,16 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>
> start_state = ((start & mask) == (base & mask));
> end_state = ((end & mask) == (base & mask));
> + inclusive = ((start < base) && (end > base));
>
> - if (start_state != end_state) {
> + if ((start_state != end_state) || inclusive) {
> /*
> * We have start:end spanning across an MTRR.
> - * We split the region into
> - * either
> - * (start:mtrr_end) (mtrr_end:end)
> - * or
> - * (start:mtrr_start) (mtrr_start:end)
> + * We split the region into either
> + * - start_state:1
> + * (start:mtrr_end) (mtrr_end:end)
> + * - end_state:1 or inclusive:1
> + * (start:mtrr_start) (mtrr_start:end)
Ok, I'm confused. Shouldn't the inclusive:1 case be
(start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
?
If so, this function would need more changes...
> * depending on kind of overlap.
> * Return the type for first region and a pointer to
> * the start of second region so that caller will
> @@ -195,7 +196,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> *repeat = 1;
> }
>
> - if ((start & mask) != (base & mask))
> + if (!start_state)
> continue;
That change actually makes the code more unreadable because you have to
go and look up what start_state was and the previous version actually
shows the check that start is within the range, exactly like it is
documented in the CPU manuals.
And I'd leave it this way because gcc is smart enough to reload the
result saved in start_state and not compute it again.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2015-05-05 at 19:11 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:36PM -0600, Toshi Kani wrote:
> > When an MTRR entry is inclusive to a requested range, i.e.
> > the start and end of the request are not within the MTRR
> > entry range but the range contains the MTRR entry entirely,
> > __mtrr_type_lookup() ignores such a case because both
> > start_state and end_state are set to zero.
> >
> > This bug can cause the following issues:
> > 1) reserve_memtype() tracks an effective memory type in case
> > a request type is WB (ex. /dev/mem blindly uses WB). Missing
> > to track with its effective type causes a subsequent request
> > to map the same range with the effective type to fail.
> > 2) pud_set_huge() and pmd_set_huge() check if a requested range
> > has any overlap with MTRRs. Missing to detect an overlap may
> > cause a performance penalty or undefined behavior.
> >
> > This patch fixes the bug by adding a new flag, 'inclusive',
> > to detect the inclusive case. This case is then handled in
> > the same way as (!start_state && end_state). With this fix,
> > __mtrr_type_lookup() handles the inclusive case properly.
> >
> > Signed-off-by: Toshi Kani <[email protected]>
> > ---
> > arch/x86/kernel/cpu/mtrr/generic.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > index 7d74f7b..a82e370 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -154,7 +154,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >
> > prev_match = 0xFF;
> > for (i = 0; i < num_var_ranges; ++i) {
> > - unsigned short start_state, end_state;
> > + unsigned short start_state, end_state, inclusive;
> >
> > if (!(mtrr_state.var_ranges[i].mask_lo & (1 << 11)))
> > continue;
> > @@ -166,15 +166,16 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >
> > start_state = ((start & mask) == (base & mask));
> > end_state = ((end & mask) == (base & mask));
> > + inclusive = ((start < base) && (end > base));
> >
> > - if (start_state != end_state) {
> > + if ((start_state != end_state) || inclusive) {
> > /*
> > * We have start:end spanning across an MTRR.
> > - * We split the region into
> > - * either
> > - * (start:mtrr_end) (mtrr_end:end)
> > - * or
> > - * (start:mtrr_start) (mtrr_start:end)
> > + * We split the region into either
> > + * - start_state:1
> > + * (start:mtrr_end) (mtrr_end:end)
> > + * - end_state:1 or inclusive:1
> > + * (start:mtrr_start) (mtrr_start:end)
>
> Ok, I'm confused. Shouldn't the inclusive:1 case be
>
> (start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
>
> ?
>
> If so, this function would need more changes...
Yes, that's how it gets separated eventually. Since *repeat is set in
this case, the code only needs to separate the first part at a time.
The 2nd part gets separated in the next call with the *repeat.
> > * depending on kind of overlap.
> > * Return the type for first region and a pointer to
> > * the start of second region so that caller will
> > @@ -195,7 +196,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> > *repeat = 1;
> > }
> >
> > - if ((start & mask) != (base & mask))
> > + if (!start_state)
> > continue;
>
> That change actually makes the code more unreadable because you have to
> go and look up what start_state was and the previous version actually
> shows the check that start is within the range, exactly like it is
> documented in the CPU manuals.
>
> And I'd leave it this way because gcc is smart enough to reload the
> result saved in start_state and not compute it again.
When I see such re-calculation, it makes me look at the code again to
see if there is a case that updates the parameters after the first
calculation... That said, I am OK as long as gcc is smart enough to
reload the value. I will put it back to the original.
Thanks,
-Toshi
On Tue, May 05, 2015 at 11:32:08AM -0600, Toshi Kani wrote:
> > Ok, I'm confused. Shouldn't the inclusive:1 case be
> >
> > (start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
> >
> > ?
> >
> > If so, this function would need more changes...
>
> Yes, that's how it gets separated eventually. Since *repeat is set in
> this case, the code only needs to separate the first part at a time.
> The 2nd part gets separated in the next call with the *repeat.
Aah, right, the caller is supposed to adjust the interval limits on
subsequent calls. Please reflect this in the comment because:
* (start:mtrr_start) (mtrr_start:end)
is misleading for inclusive:1.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2015-05-05 at 20:39 +0200, Borislav Petkov wrote:
> On Tue, May 05, 2015 at 11:32:08AM -0600, Toshi Kani wrote:
> > > Ok, I'm confused. Shouldn't the inclusive:1 case be
> > >
> > > (start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
> > >
> > > ?
> > >
> > > If so, this function would need more changes...
> >
> > Yes, that's how it gets separated eventually. Since *repeat is set in
> > this case, the code only needs to separate the first part at a time.
> > The 2nd part gets separated in the next call with the *repeat.
>
> Aah, right, the caller is supposed to adjust the interval limits on
> subsequent calls. Please reflect this in the comment because:
>
> * (start:mtrr_start) (mtrr_start:end)
>
> is misleading for inclusive:1.
Well, the comment kinda says it already, but I will try to clarify it.
/*
* We have start:end spanning across an MTRR.
* We split the region into either
* - start_state:1
* (start:mtrr_end) (mtrr_end:end)
* - end_state:1 or inclusive:1
* (start:mtrr_start) (mtrr_start:end)
* depending on kind of overlap.
* 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.
*/
Thanks,
-Toshi
On Tue, 2015-05-05 at 22:09 +0200, Borislav Petkov wrote:
> On Tue, May 05, 2015 at 01:31:32PM -0600, Toshi Kani wrote:
> > Well, the comment kinda says it already, but I will try to clarify it.
> >
> > /*
> > * We have start:end spanning across an MTRR.
> > * We split the region into either
> > * - start_state:1
> > * (start:mtrr_end) (mtrr_end:end)
> > * - end_state:1 or inclusive:1
> > * (start:mtrr_start) (mtrr_start:end)
>
> What I mean is this:
>
> * - start_state:1
> * (start:mtrr_end) (mtrr_end:end)
> * - end_state:1
> * (start:mtrr_start) (mtrr_start:end)
> * - inclusive:1
> * (start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
> *
> * depending on kind of overlap.
> *
> * Return the type of the first region and a pointer to the start
> * of next region so that caller will be advised to lookup again
> * after having adjusted start and end.
> *
> * Note: This way we handle multiple overlaps as well.
> */
>
> We add comments so that people can read them and can quickly understand
> what the function does. Not to make them parse it and wonder why
> inclusive:1 is listed together with end_state:1 which returns two
> intervals.
>
> Note that I changed the text to talk about the *next* region and not
> about the *second* region, to make it even more clear.
Thanks for the suggestion. I see your point. I will update it
accordingly.
-Toshi
On Tue, May 05, 2015 at 01:31:32PM -0600, Toshi Kani wrote:
> Well, the comment kinda says it already, but I will try to clarify it.
>
> /*
> * We have start:end spanning across an MTRR.
> * We split the region into either
> * - start_state:1
> * (start:mtrr_end) (mtrr_end:end)
> * - end_state:1 or inclusive:1
> * (start:mtrr_start) (mtrr_start:end)
What I mean is this:
* - start_state:1
* (start:mtrr_end) (mtrr_end:end)
* - end_state:1
* (start:mtrr_start) (mtrr_start:end)
* - inclusive:1
* (start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
*
* depending on kind of overlap.
*
* Return the type of the first region and a pointer to the start
* of next region so that caller will be advised to lookup again
* after having adjusted start and end.
*
* Note: This way we handle multiple overlaps as well.
*/
We add comments so that people can read them and can quickly understand
what the function does. Not to make them parse it and wonder why
inclusive:1 is listed together with end_state:1 which returns two
intervals.
Note that I changed the text to talk about the *next* region and not
about the *second* region, to make it even more clear.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--