Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1324885 > unrolled thread

[PATCH 1/5] MIPS: Bail on unsupported module relocs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/5] MIPS: Bail on unsupported module relocs Paul Burton <paul.burton@imgtec.com> - 2016-02-03 04:50 +0100
    Re: [PATCH 1/5] MIPS: Bail on unsupported module relocs James Hogan <james.hogan@imgtec.com> - 2016-02-03 13:30 +0100
    Re: [PATCH 1/5] MIPS: Bail on unsupported module relocs "Maciej W. Rozycki" <macro@imgtec.com> - 2016-02-03 13:30 +0100
      Re: [PATCH 1/5] MIPS: Bail on unsupported module relocs Paul Burton <paul.burton@imgtec.com> - 2016-02-03 17:20 +0100
        Re: [PATCH 1/5] MIPS: Bail on unsupported module relocs "Maciej W. Rozycki" <macro@imgtec.com> - 2016-02-03 18:00 +0100

#1324885 — [PATCH 1/5] MIPS: Bail on unsupported module relocs

When an unsupported reloc is encountered in a module, we currently
blindly branch to whatever would be at its entry in the reloc handler
function pointer arrays. This may be NULL, or if the unsupported reloc
has a type greater than that of the supported reloc with the highest
type then we'll dereference some value after the function pointer array
& branch to that. The result is at best a kernel oops.

Fix this by checking that the reloc type has an entry in the function
pointer array (ie. is less than the number of items in the array) and
that the handler is non-NULL, returning an error code to fail the module
load if no handler is found.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
---

 arch/mips/kernel/module-rela.c | 19 ++++++++++++++++---
 arch/mips/kernel/module.c      | 19 ++++++++++++++++---
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/module-rela.c b/arch/mips/kernel/module-rela.c
index 2b70723..769e316 100644
--- a/arch/mips/kernel/module-rela.c
+++ b/arch/mips/kernel/module-rela.c
@@ -109,9 +109,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		       struct module *me)
 {
 	Elf_Mips_Rela *rel = (void *) sechdrs[relsec].sh_addr;
+	int (*handler)(struct module *me, u32 *location, Elf_Addr v);
 	Elf_Sym *sym;
 	u32 *location;
-	unsigned int i;
+	unsigned int i, type;
 	Elf_Addr v;
 	int res;
 
@@ -134,9 +135,21 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 			return -ENOENT;
 		}
 
-		v = sym->st_value + rel[i].r_addend;
+		type = ELF_MIPS_R_TYPE(rel[i]);
+
+		if (type < ARRAY_SIZE(reloc_handlers_rela))
+			handler = reloc_handlers_rela[type];
+		else
+			handler = NULL;
 
-		res = reloc_handlers_rela[ELF_MIPS_R_TYPE(rel[i])](me, location, v);
+		if (!handler) {
+			pr_warn("%s: Unknown relocation type %u\n",
+				me->name, type);
+			return -EINVAL;
+		}
+
+		v = sym->st_value + rel[i].r_addend;
+		res = handler(me, location, v);
 		if (res)
 			return res;
 	}
diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index 1833f51..2adf572 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -197,9 +197,10 @@ int apply_relocate(Elf_Shdr *sechdrs, const char *strtab,
 		   struct module *me)
 {
 	Elf_Mips_Rel *rel = (void *) sechdrs[relsec].sh_addr;
+	int (*handler)(struct module *me, u32 *location, Elf_Addr v);
 	Elf_Sym *sym;
 	u32 *location;
-	unsigned int i;
+	unsigned int i, type;
 	Elf_Addr v;
 	int res;
 
@@ -223,9 +224,21 @@ int apply_relocate(Elf_Shdr *sechdrs, const char *strtab,
 			return -ENOENT;
 		}
 
-		v = sym->st_value;
+		type = ELF_MIPS_R_TYPE(rel[i]);
+
+		if (type < ARRAY_SIZE(reloc_handlers_rel))
+			handler = reloc_handlers_rel[type];
+		else
+			handler = NULL;
 
-		res = reloc_handlers_rel[ELF_MIPS_R_TYPE(rel[i])](me, location, v);
+		if (!handler) {
+			pr_warn("%s: Unknown relocation type %u\n",
+				me->name, type);
+			return -EINVAL;
+		}
+
+		v = sym->st_value;
+		res = handler(me, location, v);
 		if (res)
 			return res;
 	}
-- 
2.7.0

[toc] | [next] | [standalone]

#1325331

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 03, 2016 at 03:44:41AM +0000, Paul Burton wrote:
> When an unsupported reloc is encountered in a module, we currently
> blindly branch to whatever would be at its entry in the reloc handler
> function pointer arrays. This may be NULL, or if the unsupported reloc
> has a type greater than that of the supported reloc with the highest
> type then we'll dereference some value after the function pointer array
> & branch to that. The result is at best a kernel oops.
> 
> Fix this by checking that the reloc type has an entry in the function
> pointer array (ie. is less than the number of items in the array) and
> that the handler is non-NULL, returning an error code to fail the module
> load if no handler is found.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: James Hogan <james.hogan@imgtec.com>

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
> 
>  arch/mips/kernel/module-rela.c | 19 ++++++++++++++++---
>  arch/mips/kernel/module.c      | 19 ++++++++++++++++---
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/kernel/module-rela.c b/arch/mips/kernel/module-rela.c
> index 2b70723..769e316 100644
> --- a/arch/mips/kernel/module-rela.c
> +++ b/arch/mips/kernel/module-rela.c
> @@ -109,9 +109,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  		       struct module *me)
>  {
>  	Elf_Mips_Rela *rel = (void *) sechdrs[relsec].sh_addr;
> +	int (*handler)(struct module *me, u32 *location, Elf_Addr v);
>  	Elf_Sym *sym;
>  	u32 *location;
> -	unsigned int i;
> +	unsigned int i, type;
>  	Elf_Addr v;
>  	int res;
>  
> @@ -134,9 +135,21 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  			return -ENOENT;
>  		}
>  
> -		v = sym->st_value + rel[i].r_addend;
> +		type = ELF_MIPS_R_TYPE(rel[i]);
> +
> +		if (type < ARRAY_SIZE(reloc_handlers_rela))
> +			handler = reloc_handlers_rela[type];
> +		else
> +			handler = NULL;
>  
> -		res = reloc_handlers_rela[ELF_MIPS_R_TYPE(rel[i])](me, location, v);
> +		if (!handler) {
> +			pr_warn("%s: Unknown relocation type %u\n",
> +				me->name, type);
> +			return -EINVAL;
> +		}
> +
> +		v = sym->st_value + rel[i].r_addend;
> +		res = handler(me, location, v);
>  		if (res)
>  			return res;
>  	}
> diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
> index 1833f51..2adf572 100644
> --- a/arch/mips/kernel/module.c
> +++ b/arch/mips/kernel/module.c
> @@ -197,9 +197,10 @@ int apply_relocate(Elf_Shdr *sechdrs, const char *strtab,
>  		   struct module *me)
>  {
>  	Elf_Mips_Rel *rel = (void *) sechdrs[relsec].sh_addr;
> +	int (*handler)(struct module *me, u32 *location, Elf_Addr v);
>  	Elf_Sym *sym;
>  	u32 *location;
> -	unsigned int i;
> +	unsigned int i, type;
>  	Elf_Addr v;
>  	int res;
>  
> @@ -223,9 +224,21 @@ int apply_relocate(Elf_Shdr *sechdrs, const char *strtab,
>  			return -ENOENT;
>  		}
>  
> -		v = sym->st_value;
> +		type = ELF_MIPS_R_TYPE(rel[i]);
> +
> +		if (type < ARRAY_SIZE(reloc_handlers_rel))
> +			handler = reloc_handlers_rel[type];
> +		else
> +			handler = NULL;
>  
> -		res = reloc_handlers_rel[ELF_MIPS_R_TYPE(rel[i])](me, location, v);
> +		if (!handler) {
> +			pr_warn("%s: Unknown relocation type %u\n",
> +				me->name, type);
> +			return -EINVAL;
> +		}
> +
> +		v = sym->st_value;
> +		res = handler(me, location, v);
>  		if (res)
>  			return res;
>  	}
> -- 
> 2.7.0
> 

[toc] | [prev] | [next] | [standalone]

#1325332

On Wed, 3 Feb 2016, Paul Burton wrote:

> --- a/arch/mips/kernel/module-rela.c
> +++ b/arch/mips/kernel/module-rela.c
> @@ -134,9 +135,21 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  			return -ENOENT;
>  		}
>  
> -		v = sym->st_value + rel[i].r_addend;
> +		type = ELF_MIPS_R_TYPE(rel[i]);
> +
> +		if (type < ARRAY_SIZE(reloc_handlers_rela))
> +			handler = reloc_handlers_rela[type];
> +		else
> +			handler = NULL;
>  
> -		res = reloc_handlers_rela[ELF_MIPS_R_TYPE(rel[i])](me, location, v);
> +		if (!handler) {
> +			pr_warn("%s: Unknown relocation type %u\n",
> +				me->name, type);
> +			return -EINVAL;

 Hmm, this looks like a fatal error condition to me, the module won't 
load.  Why `pr_warn' rather than `pr_err' then?  Likewise in the other 
file.

  Maciej

[toc] | [prev] | [next] | [standalone]

#1325608

On Wed, Feb 03, 2016 at 12:24:38PM +0000, Maciej W. Rozycki wrote:
> On Wed, 3 Feb 2016, Paul Burton wrote:
> 
> > --- a/arch/mips/kernel/module-rela.c
> > +++ b/arch/mips/kernel/module-rela.c
> > @@ -134,9 +135,21 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> >  			return -ENOENT;
> >  		}
> >  
> > -		v = sym->st_value + rel[i].r_addend;
> > +		type = ELF_MIPS_R_TYPE(rel[i]);
> > +
> > +		if (type < ARRAY_SIZE(reloc_handlers_rela))
> > +			handler = reloc_handlers_rela[type];
> > +		else
> > +			handler = NULL;
> >  
> > -		res = reloc_handlers_rela[ELF_MIPS_R_TYPE(rel[i])](me, location, v);
> > +		if (!handler) {
> > +			pr_warn("%s: Unknown relocation type %u\n",
> > +				me->name, type);
> > +			return -EINVAL;
> 
>  Hmm, this looks like a fatal error condition to me, the module won't 
> load.  Why `pr_warn' rather than `pr_err' then?  Likewise in the other 
> file.
> 
>   Maciej

Hi Maciej,

To me fatality implies death, and nothing dies here. The module isn't
loaded but that's done gracefully & is not likely due to an error in the
kernel - it's far more likely that the module isn't valid. So to me,
warning seems appropriate rather than implying an error in the kernel.

Having said that I think it's a non-issue & don't really care either
way, so if Ralf wants it to be pr_err fine.

Thanks,
    Paul

[toc] | [prev] | [next] | [standalone]

#1325680

On Wed, 3 Feb 2016, Paul Burton wrote:

> >  Hmm, this looks like a fatal error condition to me, the module won't 
> > load.  Why `pr_warn' rather than `pr_err' then?  Likewise in the other 
> > file.
> 
> To me fatality implies death, and nothing dies here. The module isn't
> loaded but that's done gracefully & is not likely due to an error in the
> kernel - it's far more likely that the module isn't valid. So to me,
> warning seems appropriate rather than implying an error in the kernel.

 It may be bikeshedding, however these levels affect what goes to syslog 
and the console.  There are `crit', `alert' and `emerg' levels above, to 
raise more severe conditions.  As to `warn' I'd expect one on a succesful 
action made with some limitations, e.g. a compatibility mode of some kind, 
running with a performance limitation, some functionality disabled, etc.  
There's also `notice', which is lower, I'd use for normal actions that 
might require operator's attention, e.g. I'd put switching a network 
interface into the promiscuous mode there, due to its side effect on 
overall system performance.

 And I don't think it has to be a bug in the kernel to raise an `err' 
condition.  However I do agree the boundary here may be a bit fuzzy and 
code you've been changing doesn't seem consistent either.

 FWIW,

  Maciej

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web