Fix: avoid a NULL ptr dereference in HL-remote from !GV# #1866

ALTracer commented 2024年07月13日 13:30:37 +02:00 (Migrated from github.com)

Detailed description

• No new features.
• The existing problem is Read of NULL in remote.c on platforms which implement a stub for platform_target_voltage() as returning a NULL char*.
• This PR offers three possible solutions to this problem, two of them behind #if 0 because of bigger flash impact.

Before deploying a development build of BMF to stlinkv3e in RDP2 (so no inception debug) I tested my MPU setup idea on my trusty blackpill-f411ce. As I was tapping into the newfound power that is a working MPU on Cortex-M, I also felt like adding (to region 6 "uncaching" top 16 KiB out of 128 KiB of F411CE SRAM) a region 7 to catch any accesses to address 0, that is, NULL dereferences as a known UB in C. Even though libopencm3 doesn't help much, I have already had written the helper functions and tested them elsewhere, so appending two lines of register pokes was trivial.

Imagine my surprise when my mem_manage_handler triggered a reboot (as coded) not during GDB operation, but on attempting a BMDA scan/read. Adding a swlink-bluepillplus (with a cheesy 4-pin cable) revealed this (now that the debugger is present, my handler executes a coded breakpoint instead) beautiful stack backtrace:

(gdb) bt
#0 mem_manage_handler () at ../src/platforms/common/blackpill-f4/blackpill-f4.c:334
#1 <signal handler called>
#2 0x080209c6 in strlen ()
#3 0x08006128 in remote_respond_string (str=0x0, response_code=75 'K') at ../src/remote.c:97
#4 0x0800660a in remote_packet_process_general (packet_len=<optimized out>, packet=0x20001198 <pbuf> "GV") at ../src/remote.c:306
#5 remote_packet_process (packet=packet@entry=0x20001198 <pbuf> "GV", packet_length=packet_length@entry=2) at ../src/remote.c:604
#6 0x08005a64 in consume_remote_packet (packet=packet@entry=0x20001198 <pbuf> "GV", size=size@entry=1024) at ../src/gdb_packet.c:91
#7 0x08005ae4 in gdb_getpacket (packet=packet@entry=0x20001198 <pbuf> "GV", size=size@entry=1024) at ../src/gdb_packet.c:147
#8 0x08005f7c in bmp_poll_loop () at ../src/main.c:65
#9 main () at ../src/main.c:84

This was working fine with MPU disabled -- because at 0x0 on STM32 we have the 0x08000000 Internal Flash mapped, and it starts with 0x20001000 for Initial_MSP value of +4KiB, that is bytes 0x00, 0x10, 0x00, 0x20. Or, earlier, some bigger address (actual end of SRAM) divisible by 256, so that it starts with 0x00. strlen(NULL) encounters a null-terminator and returns a 0 length.

1. Robust solution: in remote_packet_process_general(), catch the NULL returned by platform and turn it into a fixed string, kinda like feeding it to %s on some libc's, e.g. "(null)"
2. Thin solution: in remote_respond_string(), catch the NULL passed from anywhere in HL-remote stack, skip the strlen & loop and just respond with EOM. A branch-if-zero should be really cheap.
3. Proper solution: fix all the platforms to return something else. Some platforms return "ABSENT!". However, this needs cooperation with command.c

There are multiple calls of this function in command.c which print nothing or print said fixed string verbatim, or print a decimal-formatted platform ADC voltage.

	if (platform_target_voltage())
		gdb_outf("Target voltage: %s\n", platform_target_voltage());

I tried to return an empty string consisting of a null-terminator, and this no longer passes a NULL pointer around, but also prints this:
Target voltage: Volt
So whatever fixed string response we choose to use, both callsites need a way to detect it (strcmp?) and suppress the print.

Your checklist for this pull request

• I've read the Code of Conduct
• I've read the guidelines for contributing to this repository
• It builds for hardware native (see Building the firmware)
• It builds as BMDA (see Building the BMDA) and should not affect it
• I've tested it to the best of my ability
• My commit messages provide a useful short description of what the commits do

Closing issues

## Detailed description * No new features. * The existing problem is Read of NULL in remote.c on platforms which implement a stub for `platform_target_voltage()` as returning a NULL char*. * This PR offers three possible solutions to this problem, two of them behind `#if 0` because of bigger flash impact. Before deploying a development build of BMF to stlinkv3e in RDP2 (so no inception debug) I tested my MPU setup idea on my trusty blackpill-f411ce. As I was tapping into the newfound power that is a working MPU on Cortex-M, I also felt like adding (to region 6 "uncaching" top 16 KiB out of 128 KiB of F411CE SRAM) a region 7 to catch any accesses to address 0, that is, NULL dereferences as a known UB in C. Even though libopencm3 doesn't help much, I have already had written the helper functions and tested them elsewhere, so appending two lines of register pokes was trivial. Imagine my surprise when my mem_manage_handler triggered a reboot (as coded) not during GDB operation, but on attempting a BMDA scan/read. Adding a swlink-bluepillplus (with a cheesy 4-pin cable) revealed this (now that the debugger is present, my handler executes a coded breakpoint instead) beautiful stack backtrace: ```c (gdb) bt #0 mem_manage_handler () at ../src/platforms/common/blackpill-f4/blackpill-f4.c:334 #1 <signal handler called> #2 0x080209c6 in strlen () #3 0x08006128 in remote_respond_string (str=0x0, response_code=75 'K') at ../src/remote.c:97 #4 0x0800660a in remote_packet_process_general (packet_len=<optimized out>, packet=0x20001198 <pbuf> "GV") at ../src/remote.c:306 #5 remote_packet_process (packet=packet@entry=0x20001198 <pbuf> "GV", packet_length=packet_length@entry=2) at ../src/remote.c:604 #6 0x08005a64 in consume_remote_packet (packet=packet@entry=0x20001198 <pbuf> "GV", size=size@entry=1024) at ../src/gdb_packet.c:91 #7 0x08005ae4 in gdb_getpacket (packet=packet@entry=0x20001198 <pbuf> "GV", size=size@entry=1024) at ../src/gdb_packet.c:147 #8 0x08005f7c in bmp_poll_loop () at ../src/main.c:65 #9 main () at ../src/main.c:84 ``` This was working fine with MPU disabled -- because at 0x0 on STM32 we have the 0x08000000 Internal Flash mapped, and it starts with 0x20001000 for Initial_MSP value of +4KiB, that is bytes 0x00, 0x10, 0x00, 0x20. Or, earlier, some bigger address (actual end of SRAM) divisible by 256, so that it starts with 0x00. `strlen(NULL)` encounters a null-terminator and returns a 0 length. 1. Robust solution: in remote_packet_process_general(), catch the NULL returned by platform and turn it into a fixed string, kinda like feeding it to %s on some libc's, e.g. "(null)" 2. Thin solution: in remote_respond_string(), catch the NULL passed from anywhere in HL-remote stack, skip the strlen & loop and just respond with EOM. A branch-if-zero should be really cheap. 3. Proper solution: fix all the platforms to return something else. Some platforms return "ABSENT!". However, this needs cooperation with command.c There are multiple calls of this function in command.c which print nothing or print said fixed string verbatim, or print a decimal-formatted platform ADC voltage. ```c if (platform_target_voltage()) gdb_outf("Target voltage: %s\n", platform_target_voltage()); ``` I tried to return an empty string consisting of a null-terminator, and this no longer passes a NULL pointer around, but also prints this: `Target voltage: Volt` So whatever fixed string response we choose to use, both callsites need a way to detect it (strcmp?) and suppress the print. ## Your checklist for this pull request * [x] I've read the [Code of Conduct](https://github.com/blackmagic-debug/blackmagic/blob/main/CODE_OF_CONDUCT.md) * [x] I've read the [guidelines for contributing](https://github.com/blackmagic-debug/blackmagic/blob/main/CONTRIBUTING.md) to this repository * [x] It builds for hardware native (see [Building the firmware](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-firmware)) * [x] It builds as BMDA (see [Building the BMDA](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-app)) *and should not affect it* * [x] I've tested it to the best of my ability * [x] My commit messages provide a useful short description of what the commits do ## Closing issues

ALTracer commented 2024年07月15日 20:24:36 +02:00 (Migrated from github.com)

I'm confused by this implementation for native. Does it implement a binary logic measurement instead of a full 12-bit analog sample?
github.com/blackmagic-debug/blackmagic@0c4f3040da/src/platforms/native/platform.c (L406-L409)
If so, then I should re-update it to say not OK/ABSENT! but Present/Absent, which is what ftdi_target_voltage() returns (there's no ADC on FTDI MPSSE)... or leave it alone.
github.com/blackmagic-debug/blackmagic@0c4f3040da/src/platforms/hosted/ftdi_bmp.c (L822-L827)
Basically f072, f3, 96b_carbon used "ABSENT!" for unimplemented, but now we're moving to "Unknown".

I'm confused by this implementation for native. Does it implement a binary logic measurement instead of a full 12-bit analog sample? https://github.com/blackmagic-debug/blackmagic/blob/0c4f3040dab8c4ec86a3a447f5369c1cdcc108f6/src/platforms/native/platform.c#L406-L409 If so, then I should re-update it to say not OK/ABSENT! but Present/Absent, which is what `ftdi_target_voltage()` returns (there's no ADC on FTDI MPSSE)... or leave it alone. https://github.com/blackmagic-debug/blackmagic/blob/0c4f3040dab8c4ec86a3a447f5369c1cdcc108f6/src/platforms/hosted/ftdi_bmp.c#L822-L827 Basically f072, f3, 96b_carbon used "ABSENT!" for unimplemented, but now we're moving to "Unknown".

dragonmux commented 2024年07月15日 21:25:29 +02:00

Prior to HW1, BMP had only voltage sense, not ADC-based voltage measurement (as you put it "binary logic measurement"). That code is determining if there is a voltage present or not on its sense pin, which is a separate thing to detection not being implemented.

As much as it does increase the footprint in Flash for this code, "Absent"/"Present" is a much better pair of terms than "OK" and "ABSENT!" are. To this end, we're happy for you to make it match the MPSSE implementation as that's the most accurate handling for HW0 native hardware. Makes everything nice and consistent.

Prior to HW1, BMP had only voltage sense, not ADC-based voltage measurement (as you put it "binary logic measurement"). That code is determining if there is a voltage present or not on its sense pin, which is a separate thing to detection not being implemented. As much as it does increase the footprint in Flash for this code, "Absent"/"Present" is a much better pair of terms than "OK" and "ABSENT!" are. To this end, we're happy for you to make it match the MPSSE implementation as that's the most accurate handling for HW0 native hardware. Makes everything nice and consistent.

ALTracer commented 2024年07月15日 22:20:22 +02:00 (Migrated from github.com)

Rebased away the wrong mass-rename and placed a separate commit for native.
Noticed one more return NULL in FTDI impl and added commit for return "Unknown" there (when no dedicated pin registered in cable structure)
Rebased to main.

Rebased away the wrong mass-rename and placed a separate commit for `native`. Noticed one more `return NULL` in FTDI impl and added commit for `return "Unknown"` there (when no dedicated pin registered in cable structure) Rebased to main.

ALTracer commented 2024年07月15日 22:25:29 +02:00 (Migrated from github.com)

Original small-scope rejected patches for archival purposes:

1. Robust solution

diff --git a/src/remote.c b/src/remote.c
index d4909917..ae9559a8 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -258,7 +258,11 @@ static void remote_packet_process_general(char *packet, const size_t packet_len)
 	(void)packet_len;
 	switch (packet[1]) {
 	case REMOTE_VOLTAGE:
-		remote_respond_string(REMOTE_RESP_OK, platform_target_voltage());
+		// Some platforms return a NULL meaning "not implemented"
+		const char *voltage = platform_target_voltage();
+		if (!voltage)
+			voltage = "(null)";
+		remote_respond_string(REMOTE_RESP_OK, voltage);
 		break;
 	case REMOTE_NRST_SET:
 		platform_nrst_set_val(packet[2] == '1');

2. Thin solution

diff --git a/src/remote.c b/src/remote.c
index d4909917..a87cc05b 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -94,6 +94,10 @@ static void remote_respond_string(const char response_code, const char *const st
 {
 	gdb_if_putchar(REMOTE_RESP, 0);
 	gdb_if_putchar(response_code, 0);
+	if (!str) { // Avoid nullptr responses
+		gdb_if_putchar(REMOTE_EOM, 1);
+		return;
+	}
 	const size_t str_length = strlen(str);
 	for (size_t idx = 0; idx < str_length; ++idx) {
 		const char chr = str[idx];

Original small-scope rejected patches for archival purposes: 1. Robust solution ```diff diff --git a/src/remote.c b/src/remote.c index d4909917..ae9559a8 100644 --- a/src/remote.c +++ b/src/remote.c @@ -258,7 +258,11 @@ static void remote_packet_process_general(char *packet, const size_t packet_len) (void)packet_len; switch (packet[1]) { case REMOTE_VOLTAGE: - remote_respond_string(REMOTE_RESP_OK, platform_target_voltage()); + // Some platforms return a NULL meaning "not implemented" + const char *voltage = platform_target_voltage(); + if (!voltage) + voltage = "(null)"; + remote_respond_string(REMOTE_RESP_OK, voltage); break; case REMOTE_NRST_SET: platform_nrst_set_val(packet[2] == '1'); ``` 2. Thin solution ```diff diff --git a/src/remote.c b/src/remote.c index d4909917..a87cc05b 100644 --- a/src/remote.c +++ b/src/remote.c @@ -94,6 +94,10 @@ static void remote_respond_string(const char response_code, const char *const st { gdb_if_putchar(REMOTE_RESP, 0); gdb_if_putchar(response_code, 0); + if (!str) { // Avoid nullptr responses + gdb_if_putchar(REMOTE_EOM, 1); + return; + } const size_t str_length = strlen(str); for (size_t idx = 0; idx < str_length; ++idx) { const char chr = str[idx]; ```