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

Value overflow on sbiret? #158

Open
leokolln opened this issue Aug 8, 2024 · 2 comments
Open

Value overflow on sbiret? #158

leokolln opened this issue Aug 8, 2024 · 2 comments

Comments

@leokolln
Copy link

leokolln commented Aug 8, 2024

Binary Encoding chapter defines sbiret.value as long.

Extension DBCN functions Console Write and Console Read first parameter num_bytes is unsigned long.

The number of bytes written/read is returned in sbiret.value.

If a value greater than LONG_MAX is used as argument for num_bytes , what is the expected behavior?

  • Overflow on sbiret.value?
  • Caller should clamp the argument to LONG_MAX?
  • Callee should only operate up to LONG_MAX bytes per call?
  • Caller should consider Binary Encoding's definition of struct sbiret{} only as a general suggestion, but each extension can have an actual different definition of it?
  • Something else?

I know the question is a bit pedantic, but maybe relevant for other/future extensions where the possible amount of data "worked on" and its reporting may be less forgiving.

I did not check the remaining documentation to see if other such cases exists...

@atishp04
Copy link
Collaborator

atishp04 commented Aug 9, 2024

The spec also says
"This is a non-blocking SBI call and it may do partial/no writes if the debug console is not able to accept more bytes."

Thus, the caller is supposed to verify the number of bytes returned and make multiple calls if it doesn't match num_bytes.

That's why #3 is what callee should do.
Cc: @avpatel @jones-drew : We should add this to the kvm-unit-test DBCN test.

I checked the openSBI implementation which allows more than LONG_MAX. So that should be fixed if my understanding above is correct.

@jones-drew
Copy link
Contributor

The spec also says "This is a non-blocking SBI call and it may do partial/no writes if the debug console is not able to accept more bytes."

Thus, the caller is supposed to verify the number of bytes returned and make multiple calls if it doesn't match num_bytes.

That's why #3 is what callee should do. Cc: @avpatel @jones-drew : We should add this to the kvm-unit-test DBCN test.

Yup, I got Cade to implement the basic write buffer test with a loop on num_bytes. Also, I started writing a couple more tests to see what happens when we cross page boundaries and, for 32-bit, what happens when we use addresses > 4G or cross from under 4G address ranges into 4G address ranges (that found a couple opensbi bugs that I still need to patch)

I checked the openSBI implementation which allows more than LONG_MAX. So that should be fixed if my understanding above is correct.

The spec doesn't say anything about limits on num_bytes, which means up to ULONG_MAX being allowed is implied. So SBI implementations should ensure that base_addr + num_bytes isn't greater than HIGHEST_PHYSICAL_ADDRESS and return INVALID_PARAM if it is.

I'm not sure about #3, I feel like using unsigned long for the parameters at the SBI interface level, just as the ecall uses xlen wide registers, is fine. The expected limits on each parameter should be specified though, allowing SBI implementations to know how to validate the input registers and S-mode to implement wrappers which use appropriate types.

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

No branches or pull requests

3 participants