| Message ID | 20180222102407.13542-2-apw@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [trusty/master-next,1/1] UBUNTU: SAUCE: x86, extable: fix uaccess fixup detection | expand |
On 22/02/18 10: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@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) { > + /* > + * If the code and its fixup are "very far apart" then > + * they are infact tagged as uaccess'es. Handle them > + * specially and fix the fixup address. This relies on > + * the .fixup section being at higher addresses that the > + * original code. > + */ > + if (new_ip - insn_ip >= 0x7ffffff0) { > /* Special hack for uaccess_err */ > current_thread_info()->uaccess_err = 1; > new_ip -= 0x7ffffff0; > @@ -53,13 +62,16 @@ int fixup_exception(struct pt_regs *regs) > int __init early_fixup_exception(unsigned long *ip) > { > const struct exception_table_entry *fixup; > + unsigned long insn_ip; > unsigned long new_ip; > > fixup = search_exception_tables(*ip); > if (fixup) { > + insn_ip = ex_insn_addr(fixup); > new_ip = ex_fixup_addr(fixup); > > - if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) { > + /* See fixup_exception for details ... */ > + if (new_ip - insn_ip >= 0x7ffffff0) { > /* uaccess handling not supported during early boot */ > return 0; > } > We discussed this fix quite a bit yesterday, looks good to me. Thanks for fixing this Andy. Acked-by: Colin Ian King <colin.king@canonical.com>
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@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... -Stefan > + /* > + * If the code and its fixup are "very far apart" then > + * they are infact tagged as uaccess'es. Handle them > + * specially and fix the fixup address. This relies on > + * the .fixup section being at higher addresses that the > + * original code. > + */ > + if (new_ip - insn_ip >= 0x7ffffff0) { > /* Special hack for uaccess_err */ > current_thread_info()->uaccess_err = 1; > new_ip -= 0x7ffffff0; > @@ -53,13 +62,16 @@ int fixup_exception(struct pt_regs *regs) > int __init early_fixup_exception(unsigned long *ip) > { > const struct exception_table_entry *fixup; > + unsigned long insn_ip; > unsigned long new_ip; > > fixup = search_exception_tables(*ip); > if (fixup) { > + insn_ip = ex_insn_addr(fixup); > new_ip = ex_fixup_addr(fixup); > > - if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) { > + /* See fixup_exception for details ... */ > + if (new_ip - insn_ip >= 0x7ffffff0) { > /* uaccess handling not supported during early boot */ > return 0; > } >
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@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
On 02/22/18 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@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) { > + /* > + * If the code and its fixup are "very far apart" then > + * they are infact tagged as uaccess'es. Handle them > + * specially and fix the fixup address. This relies on > + * the .fixup section being at higher addresses that the > + * original code. > + */ > + if (new_ip - insn_ip >= 0x7ffffff0) { > /* Special hack for uaccess_err */ > current_thread_info()->uaccess_err = 1; > new_ip -= 0x7ffffff0; > @@ -53,13 +62,16 @@ int fixup_exception(struct pt_regs *regs) > int __init early_fixup_exception(unsigned long *ip) > { > const struct exception_table_entry *fixup; > + unsigned long insn_ip; > unsigned long new_ip; > > fixup = search_exception_tables(*ip); > if (fixup) { > + insn_ip = ex_insn_addr(fixup); > new_ip = ex_fixup_addr(fixup); > > - if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) { > + /* See fixup_exception for details ... */ > + if (new_ip - insn_ip >= 0x7ffffff0) { > /* uaccess handling not supported during early boot */ > return 0; > } > Applied to trusty/master-next, adding the missing BugLink reference on the commit message. Thanks, Kleber
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) { + /* + * If the code and its fixup are "very far apart" then + * they are infact tagged as uaccess'es. Handle them + * specially and fix the fixup address. This relies on + * the .fixup section being at higher addresses that the + * original code. + */ + if (new_ip - insn_ip >= 0x7ffffff0) { /* Special hack for uaccess_err */ current_thread_info()->uaccess_err = 1; new_ip -= 0x7ffffff0; @@ -53,13 +62,16 @@ int fixup_exception(struct pt_regs *regs) int __init early_fixup_exception(unsigned long *ip) { const struct exception_table_entry *fixup; + unsigned long insn_ip; unsigned long new_ip; fixup = search_exception_tables(*ip); if (fixup) { + insn_ip = ex_insn_addr(fixup); new_ip = ex_fixup_addr(fixup); - if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) { + /* See fixup_exception for details ... */ + if (new_ip - insn_ip >= 0x7ffffff0) { /* uaccess handling not supported during early boot */ return 0; }
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@canonical.com> --- arch/x86/mm/extable.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)