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

[spi_device] Incoming address size cannot be validated #16411

Closed
a-will opened this issue Nov 17, 2022 · 6 comments
Closed

[spi_device] Incoming address size cannot be validated #16411

a-will opened this issue Nov 17, 2022 · 6 comments
Assignees
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:spi_device Priority:P3 Priority: low

Comments

@a-will
Copy link
Contributor

a-will commented Nov 17, 2022

In flash/passthrough modes, the spi_device IP does not give any indication of how long the transaction actually was, so transactions using the wrong address mode may pass through. This might only be of interest for invalidating shortened Sector Erase transactions, where 3 bytes of address was provided when the address mode required 4 bytes of address.

This is tagged for a future release. It falls into the category of robustness in an environment with errors, but it is not critical for the current stepping.

Effort estimate by @hcallahan-lowrisc:

I'm not super confident on this estimate and hence I have leaned towards pessimism. If it is not deferred, recommend to re-evaluate.

estimate 16
remaining 2023-03-23 16

@a-will a-will added IP:spi_device Type:FutureRelease Not relevant to currently planned releases/milestones labels Nov 17, 2022
@a-will a-will added this to the Backlog milestone Nov 17, 2022
@tjaychen
Copy link

just curious, wouldn't this get picked up through payload length or something?
ie, after we get the CSb rise, and software comes to look what was received and discover it's one byte short?

@a-will
Copy link
Contributor Author

a-will commented Nov 29, 2022

@tjaychen There is no payload for Sector Erase, though :)

And you can't validate the payload length for commands with variable payload lengths.

@tjaychen
Copy link

Ah, thanks for the clarification.

@andreaskurth
Copy link
Contributor

Triaged for spi_device. Assigning to M2.5 with Priority:P2 Priority: medium because I think we should make sure the current HW behavior is suitable for production (discuss with product team), although this is "only" about robustness in an environment with errors. If yes, defer back to FutureRelease.

@andreaskurth andreaskurth modified the milestones: Backlog, Discrete: M2.5 Feb 23, 2023
@andreaskurth andreaskurth added Priority:P2 Priority: medium Triaged and removed Type:FutureRelease Not relevant to currently planned releases/milestones labels Feb 23, 2023
@hcallahan-lowrisc
Copy link
Contributor

I'm not super confident on this estimate and hence I have leaned towards pessimism. If it is not deferred, recommend to re-evaluate.

estimate 16
remaining 2023-03-23 16

@moidx moidx added Priority:P3 Priority: low and removed Priority:P2 Priority: medium labels Apr 5, 2023
@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 7, 2023
@a-will
Copy link
Contributor Author

a-will commented Feb 13, 2024

This was actually fixed in #21120. I forgot that the address FIFO is written all at once, so there is no need to separately track the number of bytes written -- It will either be 0 or the number indicated by the address mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:spi_device Priority:P3 Priority: low
Projects
None yet
Development

No branches or pull requests

6 participants