Skip to contentSkip to conversation

x86/boot: Fix NULL dereference for missing hugepagesz/hugepages value

PATCH In review Thorsten Blum 2 participants 3 messages Mar 2 – Apr 9

Subsystems (2)
Stable branch x86
lore
Share & tools
lore

Thorsten Blum

author
#0 lore raw

PATCH 1/1 x86/boot: Fix NULL dereference for missing hugepagesz/hugepages value

In parse_gb_huge_pages(), 'val' can be NULL if '=' is missing from the boot parameter. The code passes 'val' to memparse() and simple_strtoull(), which can dereference NULL.

Reject 'hugepagesz' and 'hugepages' when no value has been provided and log a warning.

Fixes 9b912485e0e7 ("x86/boot/KASLR: Add two new functions for 1GB huge pages handling")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 arch/x86/boot/compressed/kaslr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
arch/x86/boot/compressed/kaslr.c +10 -0
index 3b0948ad449f..88ccc3b2c5aa 100644
@@ -205,6 +205,11 @@ static void parse_gb_huge_pages(char *param, char *val)
 	char *p;
 
 	if (!strcmp(param, "hugepagesz")) {
+		if (!val) {
+			warn("Missing value in hugepagesz= boot parameter\n");
+			return;
+		}
+
 		p = val;
 		if (memparse(p, &p) != PUD_SIZE) {
 			gbpage_sz = false;

@@ -218,6 +223,11 @@ static void parse_gb_huge_pages(char *param, char *val)
 	}
 
 	if (!strcmp(param, "hugepages") && gbpage_sz) {
+		if (!val) {
+			warn("Missing value in hugepages= boot parameter\n");
+			return;
+		}
+
 		p = val;
 		max_gb_huge_pages = simple_strtoull(p, &p, 0);
 		return;
Signature · 2 lines
Thorsten Blum <[email protected]>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4
maintainer
#1 lore raw
Quoted 32 lines · Thorsten Blum

In parse_gb_huge_pages(), 'val' can be NULL if '=' is missing from the boot parameter. The code passes 'val' to memparse() and simple_strtoull(), which can dereference NULL.

Reject 'hugepagesz' and 'hugepages' when no value has been provided and log a warning.

Fixes 9b912485e0e7 ("x86/boot/KASLR: Add two new functions for 1GB huge pages handling")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 arch/x86/boot/compressed/kaslr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
arch/x86/boot/compressed/kaslr.c +5 -0
index 3b0948ad449f..88ccc3b2c5aa 100644
@@ -205,6 +205,11 @@ static void parse_gb_huge_pages(char *param, char *val)
 	char *p;
 
 	if (!strcmp(param, "hugepagesz")) {
+		if (!val) {
+			warn("Missing value in hugepagesz= boot parameter\n");
+			return;
+		}
+
 		p = val;
 		if (memparse(p, &p) != PUD_SIZE) {
 			gbpage_sz = false;

@@ -218,6 +223,11 @@ static void parse_gb_huge_pages(char *param, char *val)
 	}

arch/x86/boot/compressed/kaslr.c static void parse_gb_huge_pages(char *param, char *val) · line 233

Quoted 10 lines
arch/x86/boot/compressed/kaslr.c +5 -0
 
 	if (!strcmp(param, "hugepages") && gbpage_sz) {
+		if (!val) {
+			warn("Missing value in hugepages= boot parameter\n");
+			return;
+		}
+
 		p = val;
 		max_gb_huge_pages = simple_strtoull(p, &p, 0);
 		return;

The intent is good even if it is not working fully yet, see below.

That's with

[    0.000000] Command line: root=/dev/sda2 resume=/dev/sda3 debug ignore_loglevel log_buf_len=16M earlyprintk=ttyS0,115200 console=ttyS0,115200 console=tty0 no_console_suspend nokaslr no_hash_pointers sysrq_always_enabled net.ifnames=0 hugepagesz

on the cmdline.

And that happens even without your kaslr.c changes because I have

# CONFIG_RANDOMIZE_BASE is not set

So it looks like there's more crap in the parsing of those two options.

Also, while at it, you probably wanna add this:

---

arch/x86/boot/compressed/kaslr.c +4 -3
index 88ccc3b2c5aa..e041be5e4326 100644
@@ -206,8 +206,8 @@ static void parse_gb_huge_pages(char *param, char *val)
 
 	if (!strcmp(param, "hugepagesz")) {
 		if (!val) {
-			warn("Missing value in hugepagesz= boot parameter\n");
-			return;
+			warn("No value supplied with hugepagesz= boot parameter\n");
+			goto next;
 		}
 
 		p = val;

@@ -222,9 +222,10 @@ static void parse_gb_huge_pages(char *param, char *val)
 		return;
 	}
 
+next:
 	if (!strcmp(param, "hugepages") && gbpage_sz) {
 		if (!val) {
-			warn("Missing value in hugepages= boot parameter\n");
+			warn("No value supplied with hugepages= boot parameter\n");
 			return;
 		}

---

I'm not sure what the logic is wrt allowing *both* cmdline options or separately or whatnot.

In any case, I'd appreciate it if you take the time and whack all those possible snafus with parsing hugepage* options so that we're solid there.

Thx!

---
PANIC: early exception 0x0e IP 10:ffffffff81d1e014 error 0 cr2 0x0

[    0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 7.0.0-rc3+ #1 PREEMPT(undef) 
[    0.000000] RIP: 0010:strlen+0x4/0x30
[    0.000000] Code: f7 75 ec 31 c0 e9 bc 6a 02 00 48 89 f8 e9 b4 6a 02 00 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa <80> 3f 00 74 18 48 89 f8 0f 1f 40 00 48 83 c0 01 80 38 00 75 f7 48
[    0.000000] RSP: 0000:ffffffff82403dc0 EFLAGS: 00010097 ORIG_RAX: 0000000000000000
[    0.000000] RAX: ffffffff899c5a70 RBX: 0000000000000000 RCX: 00000000ffffffea
[    0.000000] RDX: 0000000000000000 RSI: ffffffff899c6100 RDI: 0000000000000000
[    0.000000] RBP: 0000000000000000 R08: ffffffff899fe170 R09: 0000000000000000
[    0.000000] R10: ffffffff82403e40 R11: ffffffff82403e38 R12: ffffffff899c6100
[    0.000000] R13: 000000000000005f R14: ffffffff899fe17a R15: ffffffff899fe170
[    0.000000] FS:  0000000000000000(0000) GS:0000000000000000(0000) knlGS:0000000000000000
[    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.000000] CR2: 0000000000000000 CR3: 000000000a208000 CR4: 00000000000000f0
[    0.000000] Call Trace:
[    0.000000]  <TASK>
[    0.000000]  ? hugetlb_add_param+0x24/0x90
[    0.000000]  ? do_early_param+0x44/0x70
[    0.000000]  ? parse_args+0x146/0x410
[    0.000000]  ? _printk+0x4c/0x60
[    0.000000]  ? parse_early_options+0x29/0x30
[    0.000000]  ? __pfx_do_early_param+0x10/0x10
[    0.000000]  ? parse_early_param+0x36/0x90
[    0.000000]  ? setup_arch+0x47b/0xa90
[    0.000000]  ? _printk+0x4c/0x60
[    0.000000]  ? start_kernel+0x56/0x770
[    0.000000]  ? x86_64_start_reservations+0x24/0x30
[    0.000000]  ? x86_64_start_kernel+0xd6/0xe0
[    0.000000]  ? common_startup_64+0x13e/0x141
[    0.000000]  </TASK>
Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#2 lore raw
Quoted 34 lines · Borislav Petkov
Quoted 33 lines · Thorsten Blum

In parse_gb_huge_pages(), 'val' can be NULL if '=' is missing from the boot parameter. The code passes 'val' to memparse() and simple_strtoull(), which can dereference NULL.

Reject 'hugepagesz' and 'hugepages' when no value has been provided and log a warning.

Fixes 9b912485e0e7 ("x86/boot/KASLR: Add two new functions for 1GB huge pages handling")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 arch/x86/boot/compressed/kaslr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
arch/x86/boot/compressed/kaslr.c +5 -0
index 3b0948ad449f..88ccc3b2c5aa 100644
@@ -205,6 +205,11 @@ static void parse_gb_huge_pages(char *param, char *val)
 	char *p;
 
 	if (!strcmp(param, "hugepagesz")) {
+		if (!val) {
+			warn("Missing value in hugepagesz= boot parameter\n");
+			return;
+		}
+
 		p = val;
 		if (memparse(p, &p) != PUD_SIZE) {
 			gbpage_sz = false;

@@ -218,6 +223,11 @@ static void parse_gb_huge_pages(char *param, char *val)
 	}
Quoted 11 lines

arch/x86/boot/compressed/kaslr.c static void parse_gb_huge_pages(char *param, char *val) · line 233

Quoted 9 lines
arch/x86/boot/compressed/kaslr.c +5 -0
 	if (!strcmp(param, "hugepages") && gbpage_sz) {
+		if (!val) {
+			warn("Missing value in hugepages= boot parameter\n");
+			return;
+		}
+
 		p = val;
 		max_gb_huge_pages = simple_strtoull(p, &p, 0);
 		return;

The intent is good even if it is not working fully yet, see below.

I fixed this with [*], which prevents parse_gb_huge_pages() from being called with a NULL pointer in the first place. Please drop this patch.

Quoted 1 line

[...]

Thanks,
Thorsten

[*] https://lore.kernel.org/lkml/[email protected]/