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

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
Related show

Commit Message

Andy Whitcroft Feb. 22, 2018, 10:24 a.m. UTC
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(-)

Comments

Colin King Feb. 22, 2018, 10:59 a.m. UTC | #1
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>
Stefan Bader Feb. 23, 2018, 9:04 a.m. UTC | #2
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;
>  		}
>
Andy Whitcroft Feb. 25, 2018, 2:26 p.m. UTC | #3
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
Kleber Souza Feb. 27, 2018, 9:08 a.m. UTC | #4
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

Patch
diff mbox series

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;
 		}