Page MenuHomeFreeBSD

amd64: Relax the assertion added in commit 4a59cbc12
ClosedPublic

Authored by markj on Jun 1 2021, 2:19 PM.

Details

Summary

We only need to ensure that interrupts are disabled when handling a
fault from iret. Otherwise it's possible to trigger the assertion
legitimately, e.g., by copying in from an invalid address.

Fixes: 4a59cbc12
Reported by: pho

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Jun 1 2021, 2:19 PM

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

This revision is now accepted and ready to land.Jun 1 2021, 2:43 PM
In D30594#687126, @kib wrote:

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

Yeah, that's why I wrote it this way originally. I missed that we handle pcb_onfault != NULL in the same block.

I can add the same assertion to all of the segment load fault cases. Do you see a better approach?

In D30594#687126, @kib wrote:

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

Yeah, that's why I wrote it this way originally. I missed that we handle pcb_onfault != NULL in the same block.

I can add the same assertion to all of the segment load fault cases. Do you see a better approach?

No, I think explicitly mark all cases with asserts is the only way. Or it could be a table of addresses and new %rips for segment faults, with one loop iterating over, and one assert in case of the match. But I think it is not needed (yet).

Add assertions for segment register update faults. Convert to using a table.

This revision now requires review to proceed.Jun 1 2021, 5:20 PM

Actually I am having trouble convincing myself that

In D30594#687163, @kib wrote:
In D30594#687126, @kib wrote:

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

Yeah, that's why I wrote it this way originally. I missed that we handle pcb_onfault != NULL in the same block.

I can add the same assertion to all of the segment load fault cases. Do you see a better approach?

No, I think explicitly mark all cases with asserts is the only way. Or it could be a table of addresses and new %rips for segment faults, with one loop iterating over, and one assert in case of the match. But I think it is not needed (yet).

I converted to a table, it seemed quite ugly to duplicate the assertion for each case. I tested with a program that uses setcontext() to set bogus segment register values.

This revision is now accepted and ready to land.Jun 1 2021, 9:36 PM