Can't attach to Access Port on nRF52

[giobauermeister]

I'm doing some tests with v1.9.0-rc1 and found that I can't attach to Access Port on nRF52.

I tested v1.8.2 5cd4306 and v1.9.0-rc0 14e9df6. They seem to be working fine.

(gdb) target extended-remote /dev/ttyBmpGdb
Remote debugging using /dev/ttyBmpGdb
(gdb) monitor swdp_scan
Target voltage: 3.18V
Available Targets:
No. Att Driver
 1      Nordic nRF52 M4
 2      Nordic nRF52 Access Port 
(gdb) att 2
Attaching to Remote target
Ignoring packet error, continuing...

[zyp]

I can reproduce this with BMDA:

(gdb) target ext :2000
Remote debugging using :2000
(gdb) mon swdp_scan
Available Targets:
No. Att Driver
 1      Nordic nRF52 M4
 2      Nordic nRF52 Access Port 
(gdb) attach 2
Attaching to Remote target
Remote connection closed

BMDA is segfaulting:

Black Magic Debug App v1.9.0-rc1
 for Black Magic Probe, ST-Link v2 and v3, CMSIS-DAP, JLink and libftdi/MPSSE
Using 1209:3443 2FCC8092A2189006 Orbcode
 Orbtrace
Listening on TCP port: 2000
scan_multidrop: false
Segmentation fault: 11

[zyp]

The culprit appears to be 2c52af6.

regs_description is not set for the access port, so exec_q_feature_read attempts calling a null function pointer.

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000000000000
    frame #1: 0x000000010000b2b9 blackmagic`exec_q_feature_read(packet="0,3fb", length=<unavailable>) at gdb_main.c:475:34 [opt]
    frame #2: 0x000000010000a8e1 blackmagic`gdb_main_loop at gdb_main.c:383:4 [opt]
    frame #3: 0x000000010000a8d2 blackmagic`gdb_main_loop [inlined] handle_q_packet at gdb_main.c:549 [opt]
    frame #4: 0x000000010000a8d2 blackmagic`gdb_main_loop(tc=0x000000010003b0c0, in_syscall=false) at gdb_main.c:357 [opt]
    frame #5: 0x000000010000fb67 blackmagic`main(argc=<unavailable>, argv=<unavailable>) at main.c:45:4 [opt]
    frame #6: 0x00007fff592423d5 libdyld.dylib`start + 1
(lldb) p cur_target->regs_description
(const char *(*)(target_s *)) $2 = 0x0000000000000000

[dragonmux]

Ack, we'll check the other special AP functions (this isn't the only one) and verify they set the target structure up properly. We're surprised this worked before as we replaced the tdesc stuff like-for-like - we'll dive into a suitable way to fix this, even if that ends up being an if guard in target_regs_description:

blackmagic/src/target/target.c

Lines 471 to 474 in 9891514

	const char *target_regs_description(target_s *t)
	{
	return t->regs_description(t);
	}

[zyp]

We're surprised this worked before as we replaced the tdesc stuff like-for-like…

It had an if-guard in the form of a ternary before:

return t->tdesc ? t->tdesc : "";

[dragonmux]

Right, but t->regs_description() can return NULL (for allocation failure) and we cannot return "" there as free("") == crash which is why the calling code should do NULL return translation now. That does suggest it should be an if statement guarding the call to regs_description() and a fall-back return NULL; though - but we need to check that results in suitable behaviour or if the correct fix is actually to set t->regs_description for those fallback APs all the same

[zyp]

I figure that in any case returning an empty target description to gdb is reasonable since there's no CPU attached to that AP. There's then two natural solutions to this problem; either we populate regs_description with a dummy function that mallocs and returns an empty string (i.e. only the end terminator) or we allow it to be null, make target_regs_description return a null string pointer when regs_description is a null function pointer and check for null in exec_q_feature_read.

I think both are equally valid solutions, with a fairly even tradeoff (cost of malloc vs cost of extra checking), so I suggest going with whichever seems to have less complexity and maintenance overhead and is least likely to cause surprises in the future.

[dragonmux]

We principally agree.. avoiding the extra malloc() is a good idea though because of heap fragmentation and overheads.. a checking path would be cheaper and more robust long-term. We'll check this results in OK behaviour in probably about an hour and open a PR to fix this issue at that point.. in the mean time, if you want to test it yourself, changing this

blackmagic/src/target/target.c

Lines 471 to 474 in 9891514

	const char *target_regs_description(target_s *t)
	{
	return t->regs_description(t);
	}

to

const char *target_regs_description(target_s *t) 
{
	if (t->regs_description)
		return t->regs_description(t); 
	return NULL;
}

will allow you to test our most likely patch and continue working

[dragonmux]

This should now be fixed - please see the linked PR

[zyp]

PR fixes the original issue, but now it's segfaulting at a different point instead, which doesn't seem to have been introduced with a recent change, but is likely platform dependent.

It's now crashing when GDB sends a p packet:

		case 'p': { /* Read single register */
			ERROR_IF_NO_TARGET();
			uint32_t reg;
			sscanf(pbuf, "p%" SCNx32, &reg);
			uint8_t val[8];
			size_t s = target_reg_read(cur_target, reg, val, sizeof(val));
			if (s > 0)
				gdb_putpacket(hexify(pbuf, val, s), s * 2U);
			else
				gdb_putpacketz("EFF");
			break;
		}

It's crashing because target_reg_read returns garbage (4295094017 in my case), which causes a buffer overflow in hexify. This happens because reg_read defaults to nop_function, which is implemented like this:

static bool nop_function()
{
	return true;
}

… and gets compiled to this:

blackmagic`nop_function:
    0x10001ef0a <+0>: pushq  %rbp
    0x10001ef0b <+1>: movq   %rsp, %rbp
    0x10001ef0e <+4>: movb   $0x1, %al
    0x10001ef10 <+6>: popq   %rbp
    0x10001ef11 <+7>: retq   

The issue is that since it's returning a bool, it's only setting al, i.e. the lower 8 bits of the full rax register, so when it's cast to fit into a function pointer returning ssize_t, the upper bits of the return value contains garbage.

The right solution in this case would be to use a different dummy function for reg_read (and possibly some of the other function pointers) that instead returns 0 as a ssize_t.

[dragonmux]

That's just outright wrong and should be implemented using a nop function that returns 0.. good catch!

This highlights why we've been trying to turn more warnings on as this is actually something the compiler can warn about. At least it's another simple fix. We'll open another PR about it specifically

[dragonmux]

Please give #1356 a whirl, zyp - we've done our best to properly replace and fix that undefined mess from before, which should result in smooth operation and good behaviour

Unfortunately, you'll need to apply the PR locally on top of #1355 till the former is merged.

[giobauermeister]

I tested #1356 on top of #1355 and it seems to be working now.