search

riscv / riscv-cheri

This repository contains the CHERI extension specification, adding hardware capabilities to RISC-V ISA to enable fine-grained memory protection and scalable compartmentalization.
https://jira.riscv.org/browse/RVG-148
Creative Commons Attribution 4.0 International
47 stars 28 forks source link

RV32 Bounds checking when exponent = 0 #290

Closed buxtonpaul closed 3 months ago

buxtonpaul commented 3 months ago

It appears that the specification is not quite correct in the handling of MXLEN==32 bounds when exponent is zero. MW = 10 EW = 5 Consider a case

length= 0x7 base = 0x800089d0

Breaking this down to pesbt bits B = 0x1d0 B + length = 0x1d7 T = T8,T[7:0] = 1, 0xd7 E = 0

Reconstructing T those :-

B=0x1d0 LCOut = T[7:0] < B[7:0] ? 1:0 = 0 L_MSB = T8 = 1

Reconstituting top two bits T[MW-1:MW-2] = B[MW-1:MW-2] +LCout + MSB = T[9:8] = B[9:8] + 1 = T[9:8] = 1 + 1 therefore becomes = 0x2d7 which is not correct!

I think this is due to the fact that in MXLEN==32 only 1 bit of T is lost I believe the correct behaviour should be that LCOut = T[8:0] < B[8:0] ? 1:0 = 0 T[MW-1] = B[MW-1] +LCout Note that T8 is just passed through as bit 8 of T.

If this sounds reasonable I can try and reformulate it as a MR to update the spec.

Paul

PRugg-Cap commented 3 months ago

Nice catch.

I think this is due to the fact that in MXLEN==32 only 1 bit of T is lost

Agreed.

I believe the correct behaviour should be that LCOut = T[8:0] < B[8:0] ? 1:0 = 0 T[MW-1] = B[MW-1] +LCout

I need to give this some more thought.

PRugg-Cap commented 3 months ago

I think the issue is more fundamental. The name T8 got picked up, but I really think it's meant to be L8, i.e. the MSB of the length that is normally implied to be 1 when we have an internal exponent. I was looking through the paper, but I can't actually find any reference to using a bit this way, so I'm losing track of where the idea came from so that I can check what the original intention was.

In any case, the current decoding specification makes a lot more sense when interpreting the bit that way.

We could encode the top bit of T, and amend things as you suggest. T8 is just L8 ^ B8 ^ LCOut, and then I think it's equivalent to just do the subtraction on the 9 bits and carry into the 10th. I think L8 is much clearer though!

In your example, the L8 of that cap would be 0, so it all works as expected.

To summarise, I think there's no actual bug here, but T8 should definitely get renamed to L8.

andresag01 commented 3 months ago

Fixed via #291