Array access overflow in src/os/vxworks/src/os-impl-idmap.c

[jhnphm]

Describe the bug
Array access overflow in src/os/vxworks/src/os-impl-idmap.c

To Reproduce
Enable FM in cFS on VxWorks. Streaming semTake/semGive errors should occur when OSAL_CONFIG_DEBUG_PRINTF is enabled

Expected behavior
Error printouts should not occur

Code snips
This PR adds the following lines:

osal/src/os/shared/src/osapi-idmap.c

Lines 163 to 164 in b8e9b83

	case OS_OBJECT_TYPE_OS_CONDVAR:
	return OS_MAX_CONDVARS;

This allows OS_OBJECT_TYPE_OS_CONDVAR to become a valid index for the OS_ObjectIdIteratorInit()->OS_ObjectIdTransactionInit()->OS_Lock_Global()->OS_Lock_Global_Impl() call chain. Unfortunately, OS_impl_objtype_lock_table for VxWorks does not contain an entry for OS_OBJECT_TYPE_OS_CONDVAR:

osal/src/os/vxworks/src/os-impl-idmap.c

Lines 74 to 87 in b8e9b83

	OS_impl_objtype_lock_t *const OS_impl_objtype_lock_table[OS_OBJECT_TYPE_USER] = {
	[OS_OBJECT_TYPE_UNDEFINED] = NULL,
	[OS_OBJECT_TYPE_OS_TASK] = &OS_task_table_lock,
	[OS_OBJECT_TYPE_OS_QUEUE] = &OS_queue_table_lock,
	[OS_OBJECT_TYPE_OS_COUNTSEM] = &OS_count_sem_table_lock,
	[OS_OBJECT_TYPE_OS_BINSEM] = &OS_bin_sem_table_lock,
	[OS_OBJECT_TYPE_OS_MUTEX] = &OS_mutex_table_lock,
	[OS_OBJECT_TYPE_OS_STREAM] = &OS_stream_table_lock,
	[OS_OBJECT_TYPE_OS_DIR] = &OS_dir_table_lock,
	[OS_OBJECT_TYPE_OS_TIMEBASE] = &OS_timebase_table_lock,
	[OS_OBJECT_TYPE_OS_TIMECB] = &OS_timecb_table_lock,
	[OS_OBJECT_TYPE_OS_MODULE] = &OS_module_table_lock,
	[OS_OBJECT_TYPE_OS_FILESYS] = &OS_filesys_table_lock,
	[OS_OBJECT_TYPE_OS_CONSOLE] = &OS_console_table_lock};

OS_Lock_Global_Impl/OS_Unlock_Global_Impl runs past the end of the array, which results in invalid vxids being passed to semTake()/semGive()

System observed on:

• SP0-s (Simics)
• OS: VxWorks
• Versions: CFE: 9f42688b2bf92e18c6faf9e7ce1f9a4f2ea08316, OSAL: 38559d4, PSP (gateway): f1c952121ea73b0de50eeb62ef3c0bd9d4609ca0, FM: 6ed4fc2407cfc2f2c9db97bfea2a515c5416e480 )

Additional context

Reporter Info
John N Pham, Northrop Grumman

[jphickey]

Thanks for catching this, this was an oversight in the original change. Since VxWorks 6 does not support condvars there is no real need for a lock for this objtype because it uses the "no-condvar" implementation.

The problem is that this function unconditionally tries to take the lock, regardless of whether the lock exists or not. It was assumed in OS_Lock_Global_Impl that all defined object types would have a corresponding lock, but if a particular object type is not being supported then this lock can (validly) not exist.

To fix the immediate issue I'd propose to add a null check on the table entry, and just skip the lock if so. This is OK as long as the "no-condvar" implementation is used, as that does not need a real lock.

[jphickey]

Actually, even in the case of a non-implementation it still goes through the motions of allocating a table slot (i.e. OS_ObjectIdFindNextFree is still called, it just won't actually be successful later on). This is necessary to avoid conditional compilation among other things.

So the best bet is probably to just instantiate the missing lock....