Memory protection is silently disabled on SGX

[HW42]

Changing memory protection is not possible with SGX1 (see also #172 and #503). That's a hardware limitation. But _DkVirtualMemoryProtect just returns 0 (i.e. success). This seems to be dangerous since an application might rely on that a page access will fail. So I think we should return -PAL_ERROR_NOTSUPPORT or if this is really needed for compatibility we should at least have a warning that the user should review their application.

[mkow]

The solution with warning was already implemented as a part #385. See changes to Pal/src/host/Linux-SGX/db_memory.c in that PR.
I'm afraid that we can't simply return an error, changing memory permissions is not that uncommon in software (I think).

[donporter]

I agree - although safer to return an error, this will likely break a lot of things.

I'd rather spend the effort getting SGX2/EDMM working.

Is there a particular use case you have for needing the error code?

[HW42]

The solution with warning was already implemented as a part #385.

Nice.

I'm afraid that we can't simply return an error, changing memory permissions is not that uncommon in software (I think).

I expected this.

Is there a particular use case you have for needing the error code?

No. "Just" the gut feeling that there is software that, IMO rightfully, relies on correct mprotect behavior for something security sensitive.

[HW42]

I'd rather spend the effort getting SGX2/EDMM working.

Maybe a bit off-topic, but just curious: So your plan is to drop SGX1 support, or at least make it only secondary? (I'm not trying so suggest preferences in one or the other direction, just asking what you plans here are)

[dimakuv]

As already mentioned previously, we have the warning: https://github.com/oscarlab/graphene/blob/4cb98219d8e302055587a8952c1102415a72ba42/Pal/src/host/Linux-SGX/db_memory.c#L79

We do return 0 (i.e., success), but this is better than returning an error code and breaking applications. SGX2/EDMM support is on its way (#2190), so the current solution seems fine. Closing.