sev/utils: Use iterator combinators for pvalidate and RMP revoke paths

[v-thakkar]

Switch pvalidate_range_4k() and rmp_revoke_guest_access() to iterator- based try_for_each for cleaner, idiomatic Rust.

No functional changes.

[00xc]

The changes look good to me, but I wonder if in these functions we should attempt to walk back whenever there is an error and restore state (like we do in SharedBox::try_new_zeroed). The caller essentially has no way of knowing how far we got in the loop/iterator before something failed.

[msft-jlange]

The changes look good to me, but I wonder if in these functions we should attempt to walk back whenever there is an error and restore state (like we do in SharedBox::try_new_zeroed). The caller essentially has no way of knowing how far we got in the loop/iterator before something failed.

In the case of validation, a failure to validate always indicates either a security attack from the host or a security-critical error in guest-side logic. In both cases, it is most appropriate to panic once a failure is detected so that the security posture does not erode further. The caller is not usefully equipped to handle a validation failure (or an invalidation failure) anyway. Beyond that, reverting the validation state (which is not possible on TDX at all) is no more guaranteed to succeed than the operation that failed in the first place, so reversion can never be relied upon.

In the case of page permission adjustment, there is really no reasonable case in which failure can occur at all; just like with validation, an error here either represents a security attack (if the host unexpectedly revokes a validated page) or a security-critical error in guest-side logic (if the guest erroneously forgets to validate a page before adjusting it). Following the same reason as stated above for validation, it's best just to panic on an adjustment failure rather than to try to let execution continue past the point of failure.

[00xc]

In the case of validation, a failure to validate always indicates either a security attack from the host or a security-critical error in guest-side logic. In both cases, it is most appropriate to panic once a failure is detected so that the security posture does not erode further.

When servicing SVSM_CORE_PVALIDATE guest requests via the core protocol, the spec says we are to return an error if PVALIDATE or RMPADJUST fail, so I do not think it is true that panicking is always the right choice.

Beyond that, reverting the validation state (which is not possible on TDX at all) is no more guaranteed to succeed than the operation that failed in the first place, so reversion can never be relied upon.

We can detect if the reversion fails. This is what we do in SharedBox actually: on error we first try to revert the state, and if that fails then we panic, which I think is a good middle ground.

I think at least we should let the caller know how far the operation got through if there was an error, and let them take the right action based on this information.

[joergroedel]

I do not think there is a point in undoing changes pvalidate or rmpadjust made after an error occured. The functions should return the error and let the caller handle it however appropriate in the given situation. There is one improvement, though. I think the error values for pvalidate should actually return the virtual address on which the failure happened instead of the hardware error value (which is redundant information in the error type). With the virtual address the caller has the information on how far the loop got before the error happened.

[00xc]

I do not think there is a point in undoing changes pvalidate or rmpadjust made after an error occured. The functions should return the error and let the caller handle it however appropriate in the given situation. There is one improvement, though. I think the error values for pvalidate should actually return the virtual address on which the failure happened instead of the hardware error value (which is redundant information in the error type). With the virtual address the caller has the information on how far the loop got before the error happened.

Yes, I agree. This would perhaps let us simplify the logic in SharedBox further down the line.

[v-thakkar]

I do not think there is a point in undoing changes pvalidate or rmpadjust made after an error occured. The functions should return the error and let the caller handle it however appropriate in the given situation. There is one improvement, though. I think the error values for pvalidate should actually return the virtual address on which the failure happened instead of the hardware error value (which is redundant information in the error type). With the virtual address the caller has the information on how far the loop got before the error happened.

Returning the virtual address instead of the hardware error value makes sense to me. Should I fold that change in this PR or would you prefer a separate one?

[joergroedel]

I get an early crash with these changes:

[SVSM] VMSA PA: 0x8000c40000
[SVSM] Launching Firmware
[init] COCONUT-SVSM init process starting
[SVSM] Terminating task init, exit_code 0
[SVSM] Launching request-processing task on CPU 1
[SVSM] Launching request-processing task on CPU 2
[SVSM] Launching request-processing task on CPU 3
[SVSM] Launching request-processing task on CPU 4
[SVSM] Launching request-processing task on CPU 5
[SVSM] Launching request-processing task on CPU 6
[SVSM] Launching request-processing task on CPU 7
[SVSM] Launching request-processing task on CPU 8
[SVSM] Launching request-processing task on CPU 9
[SVSM] Launching request-processing task on CPU 10
[SVSM] Launching request-processing task on CPU 11
[SVSM] Launching request-processing task on CPU 12
[SVSM] Launching request-processing task on CPU 13
[SVSM] Launching request-processing task on CPU 14
[SVSM] Launching request-processing task on CPU 15
[SVSM] Launching request-processing task on CPU 16
[SVSM] Launching request-processing task on CPU 17
[SVSM] Launching request-processing task on CPU 18
[SVSM] Launching request-processing task on CPU 19
[SVSM] Launching request-processing task on CPU 20
[SVSM] Launching request-processing task on CPU 21
[SVSM] Launching request-processing task on CPU 22
[SVSM] Launching request-processing task on CPU 0
[SVSM] Launching request-processing task on CPU 23
SecCoreStartupWithStack(0xFFFCC000, 0x820000)
EAX=00000000 EBX=00000000 ECX=00000000 EDX=00800f12
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =f000 ffff0000 0000ffff 00009b00
SS =0000 00000000 0000ffff 00009300
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000000
Code=c5 5a 08 2d 00 00 00 00 00 00 00 00 00 00 00 00 56 54 46 00 <0f> 20 c0 a8 01 74 05 e9 2c ff ff ff e9 11 ff 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Can you please have a look?

[joergroedel]

test-in-svsm passes, but Linux boot crashes early in OVMF.

[v-thakkar]

These changes have been tested with Carlos's fix in the planes branch: coconut-svsm/linux#19 + revert of the commit 5396242 in svsm. I have been able to boot the linux guest.