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

Some functions missing range check on AtsNumber #118

Closed
2 tasks done
jphickey opened this issue Oct 12, 2023 · 1 comment · Fixed by #127
Closed
2 tasks done

Some functions missing range check on AtsNumber #118

jphickey opened this issue Oct 12, 2023 · 1 comment · Fixed by #127
Assignees
Labels

Comments

@jphickey
Copy link
Contributor

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_KillAts() function does not do a range-check on the AtsNumber. In the case that the number is 0, this will end up accessing an invalid array index.

To Reproduce
This is actually done by the unit test code, it calls SC_KillAts() with the AtsNumber set to 0. However, by chance, it doesn't segfault - merely because the value is a uint8, therefore with the wrap-around it only attempts to access array index 255, which is likely to be on the same memory page. It does, however, corrupt some other memory by doing so.

Expected behavior
Should range check, should not access invalid memory locations

Code snips

SC_OperData.AtsInfoTblAddr[SC_ATS_NUM_TO_INDEX(SC_OperData.AtsCtrlBlckAddr->AtsNumber)].AtsUseCtr++;

There is a lot going on in this one line of code - it should be split up. But in particular, the SC_ATS_NUM_TO_INDEX subtracts 1, so if the AtsNumber is 0, this will end up accessing an invalid array index.

System observed on:
Debian

Additional context
The SC_SwitchAtsCmd() (and in particular the SC_ToggleAtsIndex() used by it) also have a similar problem.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the bug label Oct 12, 2023
@jphickey jphickey self-assigned this Oct 12, 2023
@jphickey
Copy link
Contributor Author

As a side note, it is worth mentioning that the corresponding AtpState is checked here. It is not valid for AtsNumber to be 0 while the AtpState is also set non-idle.

Its a case where the coverage test is invoking this function in a state where it would never be invoked during normal FSW operation.

jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Adds inline functions to do range checking, and uses them in all places
where the same logic had been copied around.  This reduces repetition of
logic.

Introduces proper data types and wrapper functions to deal with the
different types of IDs and indices.
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Adds inline functions to do range checking, and uses them in all places
where the same logic had been copied around.  This reduces repetition of
logic.

Introduces proper data types and wrapper functions to deal with the
different types of IDs and indices.
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Adds inline functions to do range checking, and uses them in all places
where the same logic had been copied around.  This reduces repetition of
logic.

Introduces proper data types and wrapper functions to deal with the
different types of IDs and indices.
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Adds inline functions to do range checking, and uses them in all places
where the same logic had been copied around.  This reduces repetition of
logic.

Introduces proper data types and wrapper functions to deal with the
different types of IDs and indices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant