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.

Sign up for GitHub

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

Jump to bottom

Application-supplied buffer addresses not checked for validity #406

Closed
wants to merge 1 commit into from
Closed

Application-supplied buffer addresses not checked for validity #406

wants to merge 1 commit into from

Conversation

tammyleino
Copy link
Collaborator

Code must check for valid buffer address to avoid potential corruption
Signed-off-by: Tammy Leino [email protected]

@tammyleino tammyleino force-pushed the open-amp_360 branch 3 times, most recently from e450d21 to eca60ff Compare July 25, 2022 20:53
edmooring
edmooring reviewed Jul 26, 2022
View reviewed changes
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make these chunks into a macro? At first glance, they are all doing the same thing.

@tammyleino
Copy link
Collaborator Author

@edmooring Yes, I can do that. Thank you.

@tammyleino
Copy link
Collaborator Author

I have not ported my API test application to OpenAMP, but here are the tests I am running against our implementation:

/*** rpmsg_hold_rx_buffer ***/

/* null rpmsg_endpoint struct */
/* null buffer address */
/* locally scoped memory */
/* one byte before the valid shared memory range */
/* within the rpmsg_hdr section */
/* one byte outside shared memory */

/*** rpmsg_release_rx_buffer ***/

/* null rpmsg_endpoint struct */
/* null buffer address */
/* locally scoped memory */
/* one byte before the valid shared memory range */
/* within the rpmsg_hdr section */
/* one byte outside shared memory */

/*** rpmsg_send_offchannel_nocopy ***/

/* null rpmsg_endpoint struct */
/* null tx buffer */
/* invalid length */
/* bad destination address */
/* use a bad buffer to send the data - locally scoped memory */
/* use a bad buffer to send the data - one byte past the valid shared memory range */
/* use a bad buffer to send the data - one byte before the valid shared memory range */
/* use a bad buffer to send the data - within the rpmsg_hdr section */

@tammyleino tammyleino mentioned this pull request Jul 28, 2022
Open
@arnopo arnopo requested review from arnopo and edmooring August 17, 2022 09:51
arnopo
arnopo requested changes Aug 17, 2022
View reviewed changes
lib/rpmsg/rpmsg_internal.h Outdated
sizeof(struct rpmsg_hdr))) && \
((addr + len) < (((struct metal_io_region *)rvdev->rvq->shm_io)->virt + \
((struct metal_io_region *)rvdev->rvq->shm_io)->size))) ? 1 : 0)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metal_io_region structure should be manipulated in libmetal.
please create metal_io_block_check or metal_io_is_block_valid API in libmetal instead

Copy link
Collaborator Author

@tammyleino tammyleino Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnopo I have some questions since this will cause a dependency for OpenAMP on a specific version of libmetal. From a PR merging standpoint, we can just ensure the two PRs in the different repos are merged at the same time, but from a release standpoint, how do we denote that OpenAMP version x is dependent on libmetal version y? Or, do we strive to keep OpenAMP interoperable with older verisons of libmetal and thus remove dependencies? In which case, I would need to #define the libmetal macro to nothing within OpenAMP if it doesn't already exist in libmetal.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question:
In term of merging, I will integrate the libmetal first, and rerun the OpenAMP build test to ensure that is working.

For the release. I don't expect to maintain interoperability with older versions of libmetal.
I think we should only have a dependency message in the release note and offer to send a PR on the dedicated branch for the release if backwards compatibility is needed for a specific libmetal version.
I assume that libmetal APIs will be independent enough to be backported to a legacy branch...
@edmooring: what is your feeling?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnopo: I agree. I think there has always been an implicit assumption that the libmetal and open amp libraries are tightly coupled. We should probably document this.

@arnopo arnopo linked an issue Aug 17, 2022 that may be closed by this pull request
Verifying buffer for zero copy routines #360
Open
@tammyleino tammyleino mentioned this pull request Oct 4, 2022
Closed
@tammyleino
Application-supplied buffer addresses not checked for validity
a30441c 
Code must check for valid buffer address to avoid potential corruption
Signed-off-by: Tammy Leino <[email protected]>
@tammyleino
Copy link
Collaborator Author

Build will fail until OpenAMP/libmetal#222 is merged, but I wanted to commit these changes so they can be reviewed in sync with the libmetal PR.

edmooring
edmooring approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go.

@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Oct 15, 2023
@github-actions github-actions bot closed this Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnopo arnopo arnopo requested changes

@edmooring edmooring edmooring approved these changes

Assignees
No one assigned
Labels
Do not merge Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verifying buffer for zero copy routines
3 participants
@tammyleino @edmooring @arnopo