Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cryptlib: Track allocation sizes to fix realloc #543

Closed
wants to merge 1 commit into from
Closed

Cryptlib: Track allocation sizes to fix realloc #543

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 51 additions & 6 deletions Cryptlib/SysCall/BaseMemAllocation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,63 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
// -- Memory-Allocation Routines --
//

/* Prefix allocations with the allocation's total size.
*
* This is used to implement `realloc`, which needs to know the
* allocation's current size so that the data can be copied over to the
* new allocation.
*/
struct AllocWithSize {
/* Size of the whole allocation, including this `size` field.
*
* Allocations returned by AllocatePool are always eight-byte aligned
* (according to the UEFI spec), so use an eight-byte `size` field to
* make the `data` field also eight-byte aligned. */
UINT64 size;

/* The beginning of the allocation returned to the caller of malloc.
*
* The pointer to this data is what will then get passed into realloc
* and free; those functions use ptr_to_alloc_with_size to get the
* pointer to the underlying pool allocation. */
UINT8 data[0];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT the shim makefiles pass -std=gnu11, so here you should likely use the flexible array member, which was introduced in C99: data[], rather than the zero-length array GCC extension.

};

/* Convert a `ptr` (as returned by malloc) to an AllocWithSize by
* subtracting eight bytes. */
static struct AllocWithSize *ptr_to_alloc_with_size (void *ptr) {
UINT8 *cptr = (UINT8*) ptr;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superfluous cast from (void*)

return (struct AllocWithSize*) (cptr - sizeof(struct AllocWithSize));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inserting a space character in a cast expression after (type-name) is a really bad habit of edk2, because the visual separation suggests that cast operators have low precedence (weak binding). In fact, cast operators have one of the strongest precedences in C (one of the tightest bindings).

Furthermore, for consistency's sake with how function calls look (in edk2 and in this patch anyway), a space character should be inserted between the sizeof operator and the (type) that follows it.

}

/* Allocates memory blocks */
void *malloc (size_t size)
{
return AllocatePool ((UINTN) size);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is a comment on old code, but anyway. If size_t differs from UINTN, then it's all busted anyway, so the cast is superfluous (both pre- and post-patch).

For example, we expect sizeof to return UINTN, too.

UINTN alloc_size = (UINTN) (size + sizeof(struct AllocWithSize));
Copy link

@lersek lersek Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above about the superfluous/misleading space after (UINTN), and the missing space after sizeof.

But much more importantly, you are introducing a potential integer overflow here. Assume the caller calculates the size argument to malloc() carefully, so that, even if that argument is ultimately based on external / untrusted data, there is no overflow in the caller. But here you increment that size with a positive integer without pre-checking for UINTN (equiv. size_t) overflow.

If the external / untrusted data source can convince the caller to pass in a large value such as (UINTN)-1 -- again, without any overflow in the caller --, alloc_size here will wrap to sizeof (struct AllocWithSize) - 1, and the AllocatePool() call below will most likely succeed.

Then you get a buffer overflow right after, when you assign alloc->size (assuming the compiler inserts no padding after the size member in the AllocWithSize structure); not to mention the buffer overflow that the caller will perform upon malloc() returning seemingly successfully, by placing up to (UINTN)-1 bytes in alloc->data.

Suggested check:

  UINTN alloc_size;

  if (size > (size_t)-1 - sizeof (struct AllocWithSize)) {
    return NULL;
  }
  alloc_size = size + sizeof (struct AllocWithSize);

Yet another observation is that, for size==0, you will allocate a buffer that can fit just struct AllocWithSize, but no data bytes. That in fact conforms to POSIX:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/malloc.html

and returning the alloc->data flexible array member, automatically cast to a pointer, is also safe per the C99 standard (6.7.2.1 Structure and union specifiers, paragraph 16 -- If this array would have no elements, it behaves as if it had one element but the behavior is undefined if any attempt is made to access that element or to generate a pointer one past it), but it definitely deserves a comment here in the code.


struct AllocWithSize *alloc = AllocatePool (alloc_size);
if (alloc) {
alloc->size = alloc_size;
return alloc->data;
} else {
return NULL;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like github lost a review comment of mine here.

Here I pointed out that this should be formatted as follows -- no else after return please:

  if (alloc) {
    alloc->size = alloc_size;
    return alloc->data;
  }
  return NULL;

}

/* Reallocate memory blocks */
void *realloc (void *ptr, size_t size)
{
//
// BUG: hardcode OldSize == size! We have no any knowledge about
// memory size of original pointer ptr.
//
return ReallocatePool (ptr, (UINTN) size, (UINTN) size);
struct AllocWithSize *alloc = ptr_to_alloc_with_size (ptr);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong; realloc() may be called with ptr=NULL. In which case it is expected to work like malloc(), for the specified size.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html

UINTN old_size = alloc->size;
Copy link

@lersek lersek Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alloc->size is a UINT64 field, but old_size is of type UINTN. Therefore, on 32-bit (IA32), this is a "truncating" conversion (well-defined, yes, but truncating).

Therefore here you should assert that alloc->size <= MAX_UINTN, before the assignment.

For good measure, also assert that alloc->size >= sizeof (struct AllocWithSize)

UINTN new_size = (UINTN) (size + sizeof(struct AllocWithSize));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, more of the same overflow and style issues as pointed out for malloc().


alloc = ReallocatePool (alloc, old_size, new_size);
Copy link

@lersek lersek Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things to consider here.

(1) At this point, we're past the malloc() shortcut (which I pointed out above, for ptr==NULL); that is, we're actually resizing. It's still possible that we're resizing to zero (size==0). In that case, the logic here will preserve a just the "empty" AllocWithSize structure. That actually does conform to POSIX, again:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html

and returning the alloc->data flexible array member again conforms to the ISO C99 standard; but again this deserves a comment in the code.

(2) Unfortunately, the other thing to consider is a nasty preexistent bug, inside ReallocatePool().

As of commit 657b248, shim consumes its own fork of gnu-efi (the submodule) at commit 03670e14f263. And at that point, the ReallocatePool function in shim/gnu-efi/lib/misc.c looks like this:

VOID *
ReallocatePool (
    IN VOID                 *OldPool,
    IN UINTN                OldSize,
    IN UINTN                NewSize
    )
{
    VOID                    *NewPool;

    NewPool = NULL;
    if (NewSize) {
        NewPool = AllocatePool (NewSize);
    }

    if (OldPool) {
        if (NewPool) {
            CopyMem (NewPool, OldPool, OldSize < NewSize ? OldSize : NewSize);
        }
    
        FreePool (OldPool);
    }
    
    return NewPool;
}

You do realize that this ReallocatePool() implementation frees OldPool even in case it attempts to, but fails, to allocate NewPool, right? That is, even in case the caller does not intend to use ReallocatePool() in a FreePool() role, but in an actual resizing role. Note that the releasing of OldPool only depends on OldPool being non-NULL on entry.

So, from our realloc() function, we're certainly calling ReallocatePool() with:

  • a nonzero old_size (see the assertion I point out above),
  • a nonzero new_size,
  • and a non-NULL "old pool" pointer (because we're going to handle ptr==NULL on its own separate path, earlier).

That means this ReallocatePool() execution, internal to our realloc() implementation, is an actual resizing operation. And if the new area cannot be allocated, we're going to trash the existent area nevertheless. We're going to propagate NULL out to the caller of realloc(), and they will think that, while realloc() surely failed, they could continue accessing the old (original) allocation. They're going to think that because that's what POSIX guarantees -- but this implementation of ReallocatePool() violates.

In other words, ReallocatePool() itself is buggy.

... It's so very demoralizing that no matter where I look in shim, I run into errors. fallback.c is full of issues (#382), and I've just briefly looked at lib/simple_file.c, and immediately spotted a leak in simple_file_open_by_handle() (namely that of root). There's basically no healthy code to build fixes upon; one doesn't even know where to start contributing patches (only for them to be ignored nearly forever). Oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your detailed review comments. I ended up closing this PR as discussed below. The new PR does not call ReallocatePool anymore.

It's so very demoralizing that no matter where I look in shim, I run into errors.

Well, we just have to fix things one step at a time :) I know it can be frustrating to find errors, but taking a step back I don't think it's fair to stay there's no healthy code to build fixes on. Just because there are additional errors that need to be fixed doesn't mean it's not worth it to go ahead and fix some bugs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment. In general I agree with that attitude. It's just that having to fix (say) three bugs as a dependency chain for the one bug you actually want to fix, is frustrating enough in itself. On top, not seeing any maintainer feedback for a good while in the discussion phase, does not evoke a lot of trust that your work, which has just grown unexpectedly in scope, will not be in vain. "One step at a time" is definitely the way to go, but once it grows into "one series at a time", maintainer overload becomes an even worse obstacle, and a strong discouragement. It makes me doubt my effort is worthwhile. But yes, in general you are right.

if (alloc) {
alloc->size = new_size;
return alloc->data;
} else {
return NULL;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same else-after-return issue as with malloc

}
}

/* De-allocates or frees a memory block */
Expand All @@ -43,6 +86,8 @@ void free (void *ptr)
// is not true of FreePool() below, so protect it.
//
if (ptr != NULL) {
ptr = ptr_to_alloc_with_size (ptr);

FreePool (ptr);
}
}