[scudo] Validate flags in MemMap and ReservedMemory methods

[fabio-d]

I've allowlisted only the flags the the underlying methods actually accept in at least one platform.

The MAP_RESIZABLE flag is only meaningful on Fuchsia and we'll probably be able to get rid of it in the new design, but we have to keep it for now as it's used by the legacy implementation.

[Chia-hungDuan]

I would suggest this kind of checks to be in the platform specific implementations

[fabio-d]

Uhm, why? I think that it's super useful to add them here, so that if you want to know if you can pass a given flag when you call these methods you only have one place to look at. If these checks were in platform-specific code, you'd have to look in three different places (and hope that they don't contradict each other).

[Chia-hungDuan]

In D152942#4422214, @fabio-d wrote:

Uhm, why? I think that it's super useful to add them here, so that if you want to know if you can pass a given flag when you call these methods you only have one place to look at. If these checks were in platform-specific code, you'd have to look in three different places (and hope that they don't contradict each other).

If a flag is used or if it's valid, only can be verified on each platform. AllowedMapFlags doesn't guarantee it'll work on each platform. Like you mentioned, MAP_RESIZABLE is only meaningful on Fuchsia so far, what if it gets passed in a wrong place, when do we know that is causing problem? I think it still needs to wait until its running on Fuchsia. If a flag is not used anymore, we should just remove it. So the AllowedMapFlags is simply the flags you have defined.

I do want to clean up the map flags in common.h, for this case, I would only move those flags to mem_map_base.h

[fabio-d]

In D152942#4422240, @Chia-hungDuan wrote:

If a flag is used or if it's valid, only can be verified on each platform. AllowedMapFlags doesn't guarantee it'll work on each platform.

Yes, AllowedMapFlags does not guarantee that each flag is supported on _every_ platform. Maybe we'll get there when we fully implement the new backends, but I agree that today it is not the case.

The idea in this CL is simply to validate that callers are not doing anything spectacularly wrong (e.g. passing garbage values) and, at the same time, document what flags platforms may consider implementing (today, if you wanted to implement a new platform backend, the only way to know for sure what flags you might receive is to grep all the callers).

The way I see it is that these are the flags that callers are allowed to pass. How/if they actually they are translated into syscalls depends on the platform.

Like you mentioned, MAP_RESIZABLE is only meaningful on Fuchsia so far, what if it gets passed in a wrong place, when do we know that is causing problem? I think it still needs to wait until its running on Fuchsia.

What I'm proposing in this CL is simply to assert that MAP_RESIZABLE is a valid flag to pass to map and remap. That's it. The actual implementation is still free to ignore it.

If linux.cpp and trusty.cpp had assertions stating that MAP_RESIZABLE is *not* acceptable, we would have to change primary64.h:772, which I think we agree is not a wrong place to use this flag. A similar line of reasoning applies to MAP_PRECOMMIT and, until D152888 lands, to MAP_ALLOWNOMEM too.

[fabio-d]

After discussing offline, I've removed the checks for map and remap and only left them for methods that only accept a subset of the flags (MemMap::setMemoryPermission and ReservedMemory::create)

[Chia-hungDuan]

I still don't think the DCHECK here is useful. As mentioned, whether a flag has a meaning depends on the platform. It doesn't bring much benefit at the abstraction layer.

[fabio-d]

Even only the checks that I've left? e.g. what's the meaning of calling setMemoryPermission with any flag except MAP_NOACCESS?

[Chia-hungDuan]

In D152942#4650944, @fabio-d wrote:

Even only the checks that I've left? e.g. what's the meaning of calling setMemoryPermission with any flag except MAP_NOACCESS?

We may think a different question first, what will happen if we call setMemoryPermission with the flags other than MAP_NOACCESS? This flags are read by bit checking and if a certain flag has no meaning on the certain platforms, it does nothing. It doesn't cause any *critical* issues. Given that the meaning of a flag is pretty platform dependent, if we do need this kind of check, it should be done at platform specific side. You may always ignore some flags on a platform. If we call setMemoryPermission with a flag that has no meaning on all platforms, then we should avoid the submission of that change at the first place.

I would suggest having this kind of checks when we have some real cases that are caused by redundant flags.

compiler-rt/lib/scudo/standalone/mem_map_base.h

bool remap(uptr Addr, uptr Size, const char *Name, uptr Flags = 0) {
     return invokeImpl(&Derived::remapImpl, Addr, Size, Name, Flags);
   }
 
   // This is used to update the pages' access permission. For example, mark
   // pages as no read/write permission.
   void setMemoryPermission(uptr Addr, uptr Size, uptr Flags) {
     DCHECK(isAllocated());
     DCHECK((Addr >= getBase()) && (Addr + Size <= getBase() + getCapacity()));
+    DCHECK_EQ(Flags & ~MAP_NOACCESS, 0);
     return static_cast<Derived *>(this)->setMemoryPermissionImpl(Addr, Size,
                                                                  Flags);
   }
 
   // Suggest releasing a set of contiguous physical pages back to the OS. Note
   // that only physical pages are supposed to be released. Any release of
   // virtual pages may lead to undefined behavior.
   void releasePagesToOS(uptr From, uptr Size) {

 template <class Derived, typename MemMapTy> class ReservedMemory {
 public:
   using MemMapT = MemMapTy;
   constexpr ReservedMemory() = default;
 
   // Reserve a chunk of memory at a suggested address.
   bool create(uptr Addr, uptr Size, const char *Name, uptr Flags = 0) {
     DCHECK(!isCreated());
+    DCHECK_EQ(Flags & ~MAP_ALLOWNOMEM, 0);
     return invokeImpl(&Derived::createImpl, Addr, Size, Name, Flags);
   }
 
   // Release the entire reserved memory.
   void release() {
     DCHECK(isCreated());
     invokeImpl(&Derived::releaseImpl);
   }