DynamoRIO / dynamorio

Dynamic Instrumentation Tool Platform
Other
2.63k stars 557 forks source link

ASSERT: "vmareas.c:3913 !dynamo_started" when running code in writable page with bad SP #6624

Open egrimley-arm opened 8 months ago

egrimley-arm commented 8 months ago

The following test program for AArch64 puts a return instruction into a writable and executable page and then calls it from an assembler helper function with SP containing the value 1. If I run it under DynamoRIO with DEBUG I get:

Internal Error: DynamoRIO debug check failure: /.../dynamorio/core/vmareas.c:3913 !dynamo_started
#include <stdint.h>
#include <stdio.h>
#include <sys/mman.h>

void call_with_bad_sp(uint32_t *);

int main()
{
    uint32_t *ret = mmap(0, 4, PROT_EXEC | PROT_READ | PROT_WRITE,
                         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    *ret = 0xd65f03c0; // RET
    __clear_cache(ret, ret + 1);
    call_with_bad_sp(ret);
    printf("Success!\n");
    return 0;
}
        .global call_with_bad_sp
call_with_bad_sp:
        mov     x20, x30
        mov     x21, sp
        mov     x1, #1
        mov     sp, x1
        blr     x0
        mov     x30, x20
        mov     sp, x21
        ret

        .section        .note.GNU-stack, "", @progbits

I haven't tested whether something similar happens on any other architecture.

Running code in writable memory with a bad stack pointer is not perhaps something that everyone wants to do every day, and perhaps this is a difficult-to-avoid limitation of DynamoRIO, but the following observations make me suspect it's a bug rather than a deliberate limitation:

So should the ASSERT(!dynamo_started) be removed or replaced with ASSERT(!dynamo_started || ...)?

EDIT: Something similar is reported here (https://groups.google.com/g/DynamoRIO-Users/c/9vmKyMSOK8M) but with no mention of a bad SP so there may be less obscure ways of reproducing this defect.

derekbruening commented 8 months ago

Probably should s/ASSERT/ASSERT_CURIOSITY/ as the dr_prepopulate_cache() case (see the comment) is an expected occurrence; otherwise it is the app doing sthg very weird, but DR should support it so should not assert fatally. Maybe downgrade to a SYSLOG_INTERNAL_WARNING.