CFE_PSP_MemRead/Write() not checking for NULL pointer args

[skliper]

In the psp/fsw/shared/cfe_psp_ram.c, we have CFE_PSP_MemRead8/16/32 and CFE_PSP_MemWrite8/16/32 functions. They all take memory addresses as uint32 values and the *Read functions take a pointer. All of these addresses & pointers are happily dereferenced without checking for a null pointer. (The -16 and -32 read/write functions DO check for alignment and error out.)

Some spot-checks revealed a larger problem: some clients that call these functions don't look at the return value.

CFE_PSP_MemRead8(): The CFS MD does check the return value in md_dwell_pkt.c, but MM does not in 10 call sites (mm_dump.c and mm_mem8.c), and cFE's ES doesn't check the status.

CFE_PSP_MemWrite8(): The CFS MMM does not check the return value in 4 call sites (mm_load.c and mm_mem8.c).

Similar results for CFE_PSP_MemRead16(), CFE_PSP_MemWrite16(), CFE_PSP_MemRead32(), and CFE_PSP_MemWrite32().

Recommendation: Since these return an error code already, these functions should check for null pointers/addresses. But ES and MM should be fixed to properly check the return codes first.

[skliper]

Imported from trac issue 30. Created by abrown4 on 2015-08-10T16:49:15, last modified: 2019-06-13T10:22:32

[skliper]

Trac comment by abrown4 on 2015-08-10 17:34:01:

On second thought, I'd add these checks even if all the downstream clients aren't checking the return values. A dereferenced null/zero argument will immediately segfault the PSP...

Unit tests available on branch 10, commit: [changeset:c3b9703].

[skliper]

Trac comment by abrown4 on 2015-08-17 15:37:24:

commit: [changeset:5b74603] Trac #34 Added null pointer arg checks to CFE_PSP_MemRead/Write().

[skliper]

Trac comment by abrown4 on 2015-08-17 15:43:06:

merged to branch #14 in commit: [changeset:2234ae6].

[skliper]

Trac comment by abrown4 on 2015-08-17 16:32:25:

Need to create tickets for MM and ES to check these return values.

[skliper]

Trac comment by sstrege on 2015-08-19 15:18:44:

MM version 2.4.0 includes return code checks on the CFE_PSP_MemRead and CFE_PSP_MemWrite call sites.

ES needs to be updated to check for CFE_PSP_MemRead and CFE_PSP_MemWrite error return values.

[skliper]

Trac comment by sstrege on 2015-08-19 15:34:34:

Verified there are no calls to CFE_PSP_MemWrite in the cFE. ES does make a call to CFE_PSP_MemRead in the CFE_ES_CalculateCRC function in cfe_es_api.c. The return code is captured but not checked. This check needs to be added. [cfs_cfe:89] has been submitted to address this.

[skliper]

Trac comment by jphickey on 2015-12-07 12:35:38:

CCB review: This has been re-extracted from #14

Commit [changeset:42e00b7], branch trac-30-memreadwrite-null-pointer reflects the latest version of this (based on current development branch).

[skliper]

Trac comment by jphickey on 2015-12-07 13:23:52:

Review comments:

Recommend DO NOT accept

• Existing ram read/write functions are a flawed design to begin with and should be deprecated. At the very least, these need to be moved to a platform-specific module rather than being in the "shared" area since the assumption that any memory address is accessible by casting a pointer to this address is wrong on any platform using an MMU.
• The one justification for these functions is when application code actually does need to read/write directly from a //physical// memory address on machine that has an MMU.
• Assuming the previous bullet is the actual intended use case for these functions, then these absolutely should not reject a physical memory address of "0". While the "C" language reserves this address, hardware certainly does not, many CPUs have a valid memory starting at physical address "0".

See ticket #10 hat I wrote about this earlier.

Another note -- the mem "write" functions seem to also reject a VALUE of "0" - which is definitely incorrect.

[skliper]

Trac comment by sstrege on 2015-12-08 11:08:18:

I'm OK with these added protections for the next release. I agree future releases need to deprecate these functions from the shared code and move them to the platform specific code.

If this change is accepted, [cfs_cfe:92] needs to be addressed and included in the 6.5 release

[skliper]

Trac comment by jphickey on 2015-12-08 12:11:41:

I'm actually fairly strongly against this change set, since I have personally written C code on a CPU (texas instruments C54x series DSP) that had its local registers memory mapped starting at address "0". Therefore performing a 16-bit read/write access to address "0" was perfectly valid (I believe this was either the interrupt mask or interrupt flag register, IIRC).

Many other CPUs put their power-on reset vector here as well.

If application code asked to do a memory access to address 0, we should do as asked, and the application must handle any fallout.

[skliper]

Trac comment by abrown4 on 2015-12-08 12:24:06:

I agree with Joe's concerns, I was a bit confused by the intent behind the API on these types of wrapped C functions. I see (at least) two general use cases for memory read/write:

1. The PSP that makes these calls, or a CFS app that needs to have the ability to read and write to any address, and its probably up to the programmers to get it right.

2. Its for more generic, e.g. "user-level" CFS product line and user-created cFE applications where they shouldn't just access any memory the want and the API is intended to safeguard the system and these higher-level users/developers and keep them out of trouble.

I'm just not sure of the original intent of the functions as written and documented.

[skliper]

Trac comment by sduran on 2015-12-14 17:50:01:

I agree with the concern, if the intent is for these functions to have access to any memory address. "0" is a valid address.

[skliper]

Trac comment by jhageman on 2019-06-10 14:49:54:

Marked for CCB review - related to #10 suggest "wontfix", mark functions as deprecated)

[skliper]

Trac comment by jhageman on 2019-06-13 10:22:32:

CCB 6/12/2019 - deprecating per #10 closing as wontfix. Apps should be updated to use built in functionality.