fix: Patch stack size for prevent potential thread stack size overflow vulnerability in telemetryTxTask, telemetryRxTask

[mirusu400]

Summary

There are potential thread stack overflow in thread function telemetryTxTask, telemetryRxTask, So I patch this by changing stack size.

Details

LibrePilot/flight/modules/RadioComBridge/RadioComBridge.c

Line 51 in 8c101ad

#define TELEM_STACK_SIZE_WORDS 150

LibrePilot/flight/modules/RadioComBridge/RadioComBridge.c

Lines 171 to 176 in 8c101ad

	// Start the primary tasks for receiving/sending UAVTalk packets from the GCS.
	// These tasks are always needed at least for configuration over HID.
	xTaskCreate(telemetryTxTask, "telemetryTxTask", TELEM_STACK_SIZE_WORDS, NULL, TASK_PRIORITY, &(data->telemetryTxTaskHandle));
	xTaskCreate(telemetryRxTask, "telemetryRxTask", TELEM_STACK_SIZE_WORDS, NULL, TASK_PRIORITY, &(data->telemetryRxTaskHandle));
	#if defined(PIOS_INCLUDE_WDG) && defined(PIOS_WDG_TELEMETRYTX)
	PIOS_WDG_RegisterFlag(PIOS_WDG_TELEMETRYTX);

Cause of both this line, telemetryTxTask and telemetryRxTask allows stack size by 150 bytes, but after manually checking there might be allow 392, 232 bytes for each function and it can be an stack overflow.

Steps to reproduce

1. Change makefile and add CFLAGS, CXXFLAGS

...
# Comment this line to not make sanitize CFLAGS, CXXFLAGS
# make flags
++ # SANITIZE_GCC_VARS += CFLAGS CXXFLAGS CPPFLAGS LDFLAGS LDLIBS
$(foreach var, $(SANITIZE_GCC_VARS), $(eval $(call SANITIZE_VAR,$(var),disallowed)))

and

export CFLAGS = '-fstack-usage'
export CXXFLAGS = '-fstack-usage'

2. build again.
   Now we can get stack usage file (*.su) for each source file, So we can manually check stack size of each function.

PoC

In case of telemetryTxTask

telemetryTxTask (telemetryTxTask) 16 size
processObjEvent (processObjEvent) 128 size
updateTelemetryStats (updateTelemetryStats) 128 size
UAVObjGetData (UAVObjGetData) 0 size
UAVObjGetInstanceData (UAVObjGetInstanceData) 24 size
xQueueGiveMutexRecursive (xQueueGiveMutexRecursive) 16 size
xQueueGenericSend (xQueueGenericSend) 48 size
prvCopyDataToQueue (prvCopyDataToQueue)  16 size
vTaskPriorityDisinherit (vTaskPriorityDisinherit) 16 size

=> SUM: 392 Bytes

In case of telemetryRxTask

telemetryRxTask (telemetryRxTask) 24 size
UAVTalkProcessInputStream (UAVTalkProcessInputStream) 32 size
UAVTalkReceiveObject (UAVTalkReceiveObject)16 size
receiveObject (receiveObject) 40 size
UAVObjUnpack (UAVObjUnpack) 24 size
createInstance (createInstance) 24 size
pios_malloc (pios_malloc) 0 size
pios_general_malloc (pios_general_malloc) 16 size
msheap_alloc (msheap_alloc 32 size
msheap_free (msheap_free) 16 size
merge_region (merge_region) 8 size

=> SUM: 232 bytes

Code changed

LibrePilot/flight/modules/Telemetry/telemetry.c

Lines 187 to 202 in 8c101ad

	xTaskCreate(telemetryTxTask,
	"TelTx",
	STACK_SIZE_TX_BYTES / 4,
	&localChannel,
	TASK_PRIORITY_TX,
	&localChannel.txTaskHandle);
	PIOS_TASK_MONITOR_RegisterTask(TASKINFO_RUNNING_TELEMETRYTX,
	localChannel.txTaskHandle);
	xTaskCreate(telemetryRxTask,
	"TelRx",
	STACK_SIZE_RX_BYTES / 4,
	&localChannel,
	TASK_PRIORITY_RX,
	&localChannel.rxTaskHandle);
	PIOS_TASK_MONITOR_RegisterTask(TASKINFO_RUNNING_TELEMETRYRX,
	localChannel.rxTaskHandle);

=> Other thread functions that use same function have 800 size, so I change this thread function's stack size to 800.

[mirusu400]

And I know, this git is just mirror and not maintained, But there are no way to report any bug reports.

Forums cannot registrate (cause email verification not work), Jira and bitbucket doesn't accept issues(PRs) with guest privilege, irc and gitter chat are abandoned.. That's why I make this PR

[AIRCAP]

Thanks for putting that here. Librepilot development is pretty much dead atm. Bitbucket doesn't work because of the terms and condition changes by Atlassian - you can't even access the code anymore without an Atlassian account, they really screwed open source projects that used it. So for documentation purposes this is probably the best place.

This is interesting. RadioCOMBridge is only used on the OPLinkMini boards to forward telemetry from and to the flightside. It doesn't expose its own system alarms when in this mode, so potental stack usage warnings might have gotten unnoticed (if the canary protection is even active on that board). Nice catch!!!

[paul-jewell]

Hey Eric - hope you're well! Perhaps we should move the code to another git service, and off Bitbucket? I don't think any of the original developers of LibrePilot are doing anything with it at the moment, but at least those wishing to continue working with it have a chance to take over? I did consider updating Qt to the latest version, but - well, it's a lot of work, and I don't have the time, nor to be honest, the inclination.

[AIRCAP]

I currently don't have much spare time to help with that, but generally I agree with that. Github sounds like the best bet atm unless you have a better suggestion. I think @AlessioMorale has control over the github organisation here I think, so we should have him on board for that.

[vodavyd]

From documentation
https://www.freertos.org/Documentation/02-Kernel/04-API-references/01-Task-creation/01-xTaskCreate

• uxStackDepth
  The number of words (not bytes!) to allocate for use as the task's stack

Thus 150 words are 600 bytes in this case.