[trusty/master-next 1/1] UBUNTU: SAUCE: x86, extable: fix uaccess fixup detection

Andy Whitcroft apw at canonical.com
Sun Feb 25 14:26:18 UTC 2018


On Fri, Feb 23, 2018 at 10:04:01AM +0100, Stefan Bader wrote:
> On 22.02.2018 11:24, Andy Whitcroft wrote:
> > The existing code intends to identify a subset of fixups which need
> > special handling, uaccess related faults need to record the failure.
> > This is done by adjusting the fixup code pointer by a (random) constant
> > 0x7ffffff0.  This is detected in fixup_exception by comparing the two
> > pointers.  The intent of this code is to detect the the delta between
> > the original code and its fixup code being greater than the constant.
> > However, the code as written triggers undefined comparison behaviour.
> > In this kernel this prevents the condition triggering, leading to panics
> > when jumping to the corrupted fixup address.
> > 
> > Convert the code to better implement the intent.  Convert both of the
> > offsets to final addresses and compare the delta between those.  Also add
> > a massive comment to explain all of this including the implicit assumptions
> > on order of the segments that this comparison implies.
> > 
> > Fixes: 706276543b69 ("x86, extable: Switch to relative exception table entries")
> > Signed-off-by: Andy Whitcroft <apw at canonical.com>
> > ---
> >  arch/x86/mm/extable.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> > index 903ec1e9c326..a06be2f7f1bb 100644
> > --- a/arch/x86/mm/extable.c
> > +++ b/arch/x86/mm/extable.c
> > @@ -17,6 +17,7 @@ ex_fixup_addr(const struct exception_table_entry *x)
> >  int fixup_exception(struct pt_regs *regs)
> >  {
> >  	const struct exception_table_entry *fixup;
> > +	unsigned long insn_ip;
> >  	unsigned long new_ip;
> >  
> >  #ifdef CONFIG_PNPBIOS
> > @@ -35,9 +36,17 @@ int fixup_exception(struct pt_regs *regs)
> >  
> >  	fixup = search_exception_tables(regs->ip);
> >  	if (fixup) {
> > +		insn_ip = ex_insn_addr(fixup);
> >  		new_ip = ex_fixup_addr(fixup);
> >  
> > -		if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) {
> 
> This is probably deliberate but I am just wondering why the -4 gets dropped here...

This is because the values are stored as self relative offsets.  They
are converted to abolute values as below:

	foo_absolute = &fixup->foo + fixup->foo;

As insn and fixup are ints and placed next to each other in the
fixup structure, if we were to point them both to the same address X
they would have a sizeof(int) difference in value.  As the original code
is comparing these relative offsets it needs to take this into account.
The replacement code is convering each to its real absolute value before
comparison therefore this offset has been accounted for already.

Hope this makes sense.

-apw




More information about the kernel-team mailing list