Memory corruption bug in zephyr/modsocket/getaddrinfo

[ganboing]

In the following code, k_sem_init is called on memory allocated on stack:

micropython/ports/zephyr/modsocket.c

Lines 436 to 442 in 255d74b

	getaddrinfo_state_t state;
	// Just validate that it's int
	(void)mp_obj_get_int(port_in);
	state.port = port_in;
	state.result = mp_obj_new_list(0, NULL);
	k_sem_init(&state.sem, 0, UINT_MAX);

This will cause memory corruption if CONFIG_OBJ_CORE_SEM=y. I can consistently reproduce it on olimex_stm32_e407 board:

[00:00:24.775,000] <err> os:   Data Access Violation
[00:00:24.775,000] <err> os:   MMFAR Address: 0x0
[00:00:24.776,000] <err> os: r0/a1:  0xfffffff8  r1/a2:  0x00000000  r2/a3:  0x0000001b
[00:00:24.776,000] <err> os: r3/a4:  0xffffffff r12/ip:  0xffffffff r14/lr:  0x0802ea93
[00:00:24.776,000] <err> os:  xpsr:  0x410b0000
[00:00:24.776,000] <err> os: s[ 0]:  0x00000002  s[ 1]:  0x08021815  s[ 2]:  0x080733f2  s[ 3]:  0x0803a11b
[00:00:24.776,000] <err> os: s[ 4]:  0x080733f2  s[ 5]:  0x00000000  s[ 6]:  0x00000000  s[ 7]:  0x080617a8
[00:00:24.776,000] <err> os: s[ 8]:  0x08071c79  s[ 9]:  0x00000010  s[10]:  0x0000000b  s[11]:  0x0802e55f
[00:00:24.776,000] <err> os: s[12]:  0x080617a8  s[13]:  0x080733e7  s[14]:  0x080556f7  s[15]:  0x08073f1e
[00:00:24.776,000] <err> os: fpscr:  0x00000001
[00:00:24.776,000] <err> os: Faulting instruction address (r15/pc): 0x080002e4
[00:00:24.776,000] <err> os: >>> ZEPHYR FATAL ERROR 19: Unknown error on CPU 0
[00:00:24.776,000] <err> os: Current thread: 0x200047e8 (mp_main)
[00:00:24.785,000] <err> os: Halting system

The issue is that with the config enabled, the address of the sem will be added to a global list obj_type_sem, ref:
https://github.com/zephyrproject-rtos/zephyr/blob/8e5e9922c5c428d457568e317256894efccce078/kernel/sem.c#L68-L73. We essentially caused a use-after-free once the function getaddrinfo returns. This also reveals a potential larger problem where we need to treat each sem's lifecycle as infinite, as I don't see a zephyr interface to "destroy" a semaphore. Thus, rather than recreating a new sem each time we enters getaddrinfo function, we'll probably need a global one. Moreover, it seems the semaphore is really not needed in this function, and we can just do without -- dns_resolve_cb is called synchronously, right?

micropython/ports/zephyr/modsocket.c

Lines 443 to 451 in 255d74b

	for (int i = 2; i--;) {
	int type = (family != AF_INET6 ? DNS_QUERY_TYPE_A : DNS_QUERY_TYPE_AAAA);
	RAISE_ERRNO(dns_get_addr_info(host, type, NULL, dns_resolve_cb, &state, 3000));
	k_sem_take(&state.sem, K_FOREVER);
	if (family != 0) {
	break;
	}
	family = AF_INET6;
	}

Please let me know your thoughts. If my theory is correct, maybe we should also review other semaphore usage in port/zephyr? Thanks!

Bo

[bikeNomad]

It looks like the only other semaphore usage in the zephyr port is the per-thread mutex, which is not on the stack: https://github.com/search?q=repo%3Amicropython%2Fmicropython+%2Fk_sem_%28take%7Cgive%7Cinit%29%2F&type=code

You're right that the stack allocation of the semaphore is wrong; it should be a file-scope static instead.

It looks like the intent is to hold the semaphore until the dns_resolve_cb callback happens to avoid more than one active DNS query. I think the k_sem_take() call should happen before the dns_get_addr_info() call though; what do you think?