Re: [PATCH] alpha: potential dereference of null pointer

[Al Viro]

On Tue, Jan 17, 2023 at 06:23:47PM +0800, ye.xingchen@xxxxxxxxxx wrote:
> From: Minghao Chi <chi.minghao@xxxxxxxxxx>
> 
> The return value of kmalloc() needs to be checked.
> To avoid use of null pointer in case of the failure of alloc.
> 
> Reported-by: Zeal Robot <zealci@xxxxxxxxxx>
> Signed-off-by: Minghao Chi <chi.minghao@xxxxxxxxxx>
> Signed-off-by: ye xingchen <ye.xingchen@xxxxxxxxxx>
> ---
>  arch/alpha/kernel/module.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/alpha/kernel/module.c b/arch/alpha/kernel/module.c
> index 5b60c248de9e..5442b75a98c2 100644
> --- a/arch/alpha/kernel/module.c
> +++ b/arch/alpha/kernel/module.c
> @@ -47,6 +47,8 @@ process_reloc_for_got(Elf64_Rela *rela,
>  		}
> 
>  	g = kmalloc (sizeof (*g), GFP_KERNEL);
> +	if (!g)
> +		return;
>  	g->next = chains[r_sym].next;
>  	g->r_addend = r_addend;
>  	g->got_offset = *poffset;

NAK.  This kind of patches is strictly worse than useless.
Look at what has happened here:

1) your tool has found an indicator of possible bug.  Might or
might not be a false positive.

2) it is *NOT* a false positive - the problem caught by your
heuristics is real.  Indeed, allocation might fail and that
would result in problems.

3) solution: send a patch that would modify the code so that the
same heuristics would no longer be able to spot the problem.

Suppose it gets applied.  Is the bug fixed?  Your heuristics no
longer trigger, but what happens in the conditions that would
have triggered the original bug?  Sure, process_reloc_for_got()
returns without an oops now.  But what was it supposed to do
with the object it tried to allocate?  It might be that
"skip allocation and move on" is correct, but from the look
of that code it seems to be unlikely.

And if you look at the caller, you'll see that
	* everything we allocate gets shortly freed
	* the caller does temporary allocations of its own (also
freed later)
	* failure of the allocations in the caller translates into
"return -ENOMEM"

IOW, the caller's callers are supposed to deal with the possibility
of -ENOMEM being returned to them in such situations, which means
that we do have a reasonably safe approach - have allocation failures
in process_reloc_for_got() reported to caller and treated as "clean
up and fail with -ENOMEM there".

*IF* your variant is actually safe, you should at the very least
include the analysis and the reasons why it works.  TBH, I do not
believe that it is safe.

And no, "it doesn't oops with that change" is not a sufficient
improvement to balance "it ends up with corrupted loaded module
in the same conditions and is harder to spot on source inspection".

I would suggest something along the lines of (completely untested)
diff below:

diff --git a/arch/alpha/kernel/module.c b/arch/alpha/kernel/module.c
index 5b60c248de9e..e6ef4c5e8f95 100644
--- a/arch/alpha/kernel/module.c
+++ b/arch/alpha/kernel/module.c
@@ -25,7 +25,7 @@ struct got_entry {
 	int got_offset;
 };
 
-static inline void
+static inline int
 process_reloc_for_got(Elf64_Rela *rela,
 		      struct got_entry *chains, Elf64_Xword *poffset)
 {
@@ -35,7 +35,7 @@ process_reloc_for_got(Elf64_Rela *rela,
 	struct got_entry *g;
 
 	if (r_type != R_ALPHA_LITERAL)
-		return;
+		return 0;
 
 	for (g = chains + r_sym; g ; g = g->next)
 		if (g->r_addend == r_addend) {
@@ -47,6 +47,8 @@ process_reloc_for_got(Elf64_Rela *rela,
 		}
 
 	g = kmalloc (sizeof (*g), GFP_KERNEL);
+	if (!g)
+		return -ENOMEM;
 	g->next = chains[r_sym].next;
 	g->r_addend = r_addend;
 	g->got_offset = *poffset;
@@ -58,6 +60,7 @@ process_reloc_for_got(Elf64_Rela *rela,
 	   42 valid relocation types, and a 32-bit field.  Co-opt the
 	   bits above 256 to store the got offset for this reloc.  */
 	rela->r_info |= g->got_offset << 8;
+	return 0;
 }
 
 int
@@ -68,6 +71,7 @@ module_frob_arch_sections(Elf64_Ehdr *hdr, Elf64_Shdr *sechdrs,
 	Elf64_Rela *rela;
 	Elf64_Shdr *esechdrs, *symtab, *s, *got;
 	unsigned long nsyms, nrela, i;
+	int err;
 
 	esechdrs = sechdrs + hdr->e_shnum;
 	symtab = got = NULL;
@@ -107,12 +111,12 @@ module_frob_arch_sections(Elf64_Ehdr *hdr, Elf64_Shdr *sechdrs,
 
 	/* Examine all LITERAL relocations to find out what GOT entries
 	   are required.  This sizes the GOT section as well.  */
-	for (s = sechdrs; s < esechdrs; ++s)
+	for (err = 0, s = sechdrs; !err && s < esechdrs; ++s)
 		if (s->sh_type == SHT_RELA) {
 			nrela = s->sh_size / sizeof(Elf64_Rela);
 			rela = (void *)hdr + s->sh_offset;
-			for (i = 0; i < nrela; ++i)
-				process_reloc_for_got(rela+i, chains,
+			for (i = 0; !err && i < nrela; ++i)
+				err = process_reloc_for_got(rela+i, chains,
 						      &got->sh_size);
 		}
 
@@ -126,7 +130,7 @@ module_frob_arch_sections(Elf64_Ehdr *hdr, Elf64_Shdr *sechdrs,
 	}
 	kfree(chains);
 
-	return 0;
+	return err;
 }
 
 int