Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing validity check on command number/index in ATS processing #121

Closed
2 tasks done
jphickey opened this issue Oct 18, 2023 · 0 comments · Fixed by #127
Closed
2 tasks done

Missing validity check on command number/index in ATS processing #121

jphickey opened this issue Oct 18, 2023 · 0 comments · Fixed by #127
Assignees

Comments

@jphickey
Copy link
Contributor

jphickey commented Oct 18, 2023

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
The SC_BeginAts() and SC_JumpAtsCmd() functions walk through the ATS entries in time-sequential order, NOT in command number order. To do this, they use a supplemental lookup table, that maps the sequence number to a command number. It then (un-conditionally) uses this command number to index into another table to find where that command starts.

However, if this table has not been initialized, the command number will be invalid.

To Reproduce
Just run unit tests. The test cases already do/did this, but it was not caught. It is corrupting memory by writing beyond the end of the table.

Expected behavior
Should validate values, and not read or write beyond the end of arrays.

Code snips

SC/fsw/src/sc_atsrq.c

Lines 204 to 208 in 05dd449

CmdIndex = SC_ATS_CMD_NUM_TO_INDEX(SC_AppData.AtsTimeIndexBuffer[AtsIndex][TimeIndex]);
/* then get the entry index from the cmd index table */
EntryIndex = SC_AppData.AtsCmdIndexBuffer[AtsIndex][CmdIndex];
/* then get a pointer to the ATS entry data */
Entry = (SC_AtsEntryHeader_t *)&SC_OperData.AtsTblAddr[AtsIndex][EntryIndex];

Specifically if the AtsTimeIndexBuffer was not fully initialized, then the AtsCmdIndexBuffer will be read at index -1.

System observed on:
Debian

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey self-assigned this Oct 18, 2023
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Adds additional range checks to avoid out-of-bounds array access
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Adds additional range checks to avoid out-of-bounds array access
@dzbaker dzbaker closed this as completed in ee21c30 Dec 5, 2023
jphickey added a commit to jphickey/SC that referenced this issue Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant