2023-04-18 06:54:09

by Daniil Dulov

Subject: [PATCH] x86/kaslr: Fix potential dereference of NULL pointer.

Pointer val can have NULL value. Then its value is assigned to the pointer p.
p is dereferenced by calling strcmp().

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 4cdba14f84c9 ("x86/KASLR: Handle the memory limit specified by the 'memmap=' and 'mem=' boot options")
Signed-off-by: Daniil Dulov <[email protected]>
---
arch/x86/boot/compressed/kaslr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b92fffbe761f..51b3925d4d2d 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -291,7 +291,7 @@ static void handle_mem_options(void)
} else if (!strcmp(param, "mem")) {
char *p = val;

- if (!strcmp(p, "nopentium"))
+ if (!p || !strcmp(p, "nopentium"))
continue;
mem_size = memparse(p, &p);
if (mem_size == 0)
--
2.25.1

2023-04-18 07:46:12

by Baoquan He

Subject: Re: [PATCH] x86/kaslr: Fix potential dereference of NULL pointer.

On 04/17/23 at 11:53pm, Daniil Dulov wrote:
> Pointer val can have NULL value. Then its value is assigned to the pointer p.
> p is dereferenced by calling strcmp().
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

It's true for strcmp from lib/string.c, while may not be so true for
strcmp in arch/x86/boot/string.c which I copy at below for reference.
Here, boot/compressed/kaslr.c is using the strcmp in
arch/x86/boot/string.c. So leaving it as is or fixing it, either looks
good to me, I even prefer the former.

int strcmp(const char *str1, const char *str2)
{
const unsigned char *s1 = (const unsigned char *)str1;
const unsigned char *s2 = (const unsigned char *)str2;
int delta = 0;

while (*s1 || *s2) {
delta = *s1 - *s2;
if (delta)
return delta;
s1++;
s2++;
}
return 0;
}

>
> Fixes: 4cdba14f84c9 ("x86/KASLR: Handle the memory limit specified by the 'memmap=' and 'mem=' boot options")
> Signed-off-by: Daniil Dulov <[email protected]>
> ---
> arch/x86/boot/compressed/kaslr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index b92fffbe761f..51b3925d4d2d 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -291,7 +291,7 @@ static void handle_mem_options(void)
> } else if (!strcmp(param, "mem")) {
> char *p = val;
>
> - if (!strcmp(p, "nopentium"))
> + if (!p || !strcmp(p, "nopentium"))
> continue;
> mem_size = memparse(p, &p);
> if (mem_size == 0)
> --
> 2.25.1
>