How buffer overflow in memcpy happens in C bios_disk.h and how to fix it

How Buffer Overflow in memcpy Happens in C bios_disk.h and How to Fix It

Introduction

In the DOSBox-X codebase, a critical buffer overflow vulnerability was discovered in include/bios_disk.h at line 474. The bios_disk_read function uses memcpy to copy 512 bytes (one disk sector) from a source buffer into a destination data buffer, but the offset calculation derived from the sectnum parameter lacked sufficient validation. While the existing code checked whether the offset exceeded the buffer size, it failed to verify that the offset plus the 512-byte copy length remained within bounds — a classic off-by-one category error that left the door open to out-of-bounds memory reads.

This vulnerability is particularly dangerous because sectnum can be influenced by disk I/O requests, meaning an attacker with the ability to craft malicious disk images or manipulate sector parameters could trigger memory disclosure or denial of service.

The Vulnerability Explained

The vulnerable code in include/bios_disk.h:474 performs a sector read operation that looks conceptually like this:

// Vulnerable code (before fix)
uint32_t offset = sectnum * 512;
// Insufficient check: only validates offset, not offset + copy_size
if (offset >= buffer_size) {
    return error_code;
}
memcpy(data, buffer + offset, 512);

The problem is subtle but critical. Consider what happens when sectnum is at the boundary:

• Buffer size: 2048 bytes
• sectnum = 4 → offset = 2048
• The check offset >= buffer_size catches this case (2048 >= 2048 is true) ✓
• But sectnum = 3 → offset = 1536
• The check 1536 >= 2048 is false, so memcpy proceeds
• memcpy(data, buffer + 1536, 512) reads bytes 1536–2047 ✓

Now consider a larger buffer scenario or integer overflow:

• If sectnum = 0xFFFFFFFF, the multiplication sectnum * 512 overflows a 32-bit integer
• The resulting offset wraps around to a small value
• The bounds check passes because the wrapped value is less than buffer_size
• memcpy then reads from an arbitrary memory location relative to buffer

Attack Scenario: An attacker who can control the sectnum parameter — for example, by providing a crafted disk image with manipulated partition table entries or by exploiting a BIOS interrupt handler — can cause the emulator to:

1. Read out-of-bounds memory: Leaking adjacent heap or stack data that may contain sensitive information
2. Crash the application: Accessing unmapped memory pages triggers a segmentation fault
3. Bypass ASLR: Leaked memory addresses from adjacent allocations can defeat address space layout randomization

The regression test included in the PR demonstrates these exact scenarios:

uint32_t test_cases[] = {
    0xFFFFFFFF,  // Exploit: large sectnum causing overflow
    0x00000004,  // Boundary: exactly at buffer limit
    0x00000000   // Valid: within bounds
};

The Fix

The fix adds a proper bounds check that validates both the offset calculation and the total read range before the memcpy operation executes:

Before (vulnerable):

uint32_t offset = sectnum * 512;
if (offset >= buffer_size) {
    return error_code;
}
memcpy(data, buffer + offset, 512);

After (fixed):

uint32_t offset = sectnum * 512;
// Check for integer overflow in multiplication
if (sectnum != 0 && offset / sectnum != 512) {
    return error_code;
}
// Check that offset + copy size stays within buffer bounds
if (offset + 512 > buffer_size) {
    return error_code;
}
memcpy(data, buffer + offset, 512);

The fix addresses two distinct issues:

1. Integer overflow protection: By verifying that offset / sectnum == 512, the code detects when the multiplication has wrapped around. This prevents attackers from using large sectnum values to bypass the subsequent bounds check.

2. Complete bounds validation: The check offset + 512 > buffer_size ensures that the entire 512-byte read window falls within the buffer, not just the starting offset. This closes the gap where the old check would allow reads that started within bounds but extended past the buffer's end.

The PR also disabled a potentially problematic CI step (if: false on the Windows build run step) and updated actions/checkout from v6 to v7 across multiple workflow files, which are maintenance changes unrelated to the security fix but included in the same PR for CI stability.

Prevention & Best Practices

1. Always validate the full range, not just the start

When performing buffer copies with computed offsets, check that offset + length <= buffer_size. Never assume that a valid starting offset implies a valid read range.

2. Guard against integer overflow in offset calculations

In C, unsigned integer overflow is defined behavior (it wraps), but it's almost never what you want. Use overflow-safe arithmetic:

// Safe multiplication check
if (sectnum > (SIZE_MAX / 512)) {
    return error_code;  // Would overflow
}

3. Use size_t for buffer arithmetic

Using uint32_t for offset calculations on 64-bit systems can mask overflows. Prefer size_t which matches the platform's address space.

4. Consider using safe memory copy wrappers

Create a project-wide safe_memcpy function that takes source buffer size as a parameter and validates bounds internally:

int safe_memcpy(void *dst, size_t dst_size, const void *src, 
                size_t src_size, size_t offset, size_t count) {
    if (offset + count > src_size || count > dst_size) return -1;
    memcpy(dst, (const char*)src + offset, count);
    return 0;
}

5. Static analysis integration

Run tools like Coverity, PVS-Studio, or Semgrep in CI to catch unchecked memcpy patterns before they reach production.

Key Takeaways

• The sectnum * 512 calculation in bios_disk.h:474 could overflow a 32-bit integer, wrapping around to bypass the existing bounds check — always validate arithmetic before using the result.
• Checking only the offset without the copy length is insufficient — the old check offset >= buffer_size missed cases where the read window extended past the buffer end.
• Disk I/O emulation code is security-critical because sector numbers originate from potentially untrusted disk images that users load.
• Line 475 in the same file uses a similar pattern and should be reviewed with the same bounds-checking approach.
• The regression test with sectnum = 0xFFFFFFFF demonstrates that integer overflow is the primary exploitation vector, not just large-but-valid sector numbers.

How Orbis AppSec Detected This

• Source: The sectnum parameter passed to the BIOS disk read function, which originates from disk image metadata or emulated BIOS interrupt calls
• Sink: memcpy(data, buffer + offset, 512) in include/bios_disk.h:474
• Missing control: No validation that (sectnum * 512) + 512 stays within the source buffer bounds, and no integer overflow check on the multiplication
• CWE: CWE-120 (Buffer Copy without Checking Size of Input)
• Fix: Added bounds check verifying that the computed offset plus 512 bytes does not exceed the buffer size, with integer overflow detection on the sector-to-offset multiplication

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

Buffer overflows in memcpy operations remain one of the most dangerous vulnerability classes in C code. This specific instance in bios_disk.h demonstrates how a seemingly reasonable bounds check can be insufficient when it doesn't account for the full copy range or integer overflow in offset calculations. The fix is straightforward — validate the complete read window and check for arithmetic overflow — but the consequences of missing it are severe: memory disclosure, crashes, and potential code execution. When writing low-level buffer manipulation code, always ask: "Does my check cover the entire operation, not just the starting point?"

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP Buffer Overflow Prevention Cheat Sheet
• C11 Standard — memcpy specification
• Semgrep rules for buffer overflow
• fix: add bounds check before memcpy in bios_disk.h