Skip to content

Resolve buffer overflow#34

Open
ChuWeiChang wants to merge 1 commit intosysprog21:mainfrom
ChuWeiChang:address-memory-leak-of-ifr-and-rt
Open

Resolve buffer overflow#34
ChuWeiChang wants to merge 1 commit intosysprog21:mainfrom
ChuWeiChang:address-memory-leak-of-ifr-and-rt

Conversation

@ChuWeiChang
Copy link
Copy Markdown

@ChuWeiChang ChuWeiChang commented Mar 30, 2026 •
edited by cubic-dev-ai bot
Loading

This PR addresses the buffer overflow reported in #33.
ifr and rt structures` is modified to match the standard Linux kernel definitions, ensuring that memory is correctly mapped.

Summary by cubic

Fixes ASan crashes from buffer overflows in network setup by replacing custom ifr/rt with Linux struct ifreq/struct rtentry and using IPv4 sockaddr_in for address, netmask, and default route. Ensures correct ABI on 64-bit and resolves #33.

  • Bug Fixes
    • Include <net/if.h>/<net/route.h> and remove hardcoded structs; previous ifreq size (32) and missing rt fields caused an 8-byte overflow and stack corruption.
    • Set ifr.ifr_addr via sockaddr_in and initialize routes with AF_INET; add _Static_assert checks (ifreq=40, rtentry=120) to guard ABI.

Written for commit 000008f. Summary will update on new commits.

jserv
jserv requested changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Read https://cbea.ms/git-commit/ carefully and enforce the rules.

cubic-dev-ai[bot]

This comment was marked as resolved.

Sign in to view

@jserv jserv changed the title Address memory leak of ifr and rt (#33) Resolve memory leaks Mar 30, 2026
@ChuWeiChang ChuWeiChang force-pushed the address-memory-leak-of-ifr-and-rt branch from ac032d9 to c835321 Compare March 31, 2026 05:32
jserv
jserv requested changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jserv jserv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Do make install-hooks, squash commits, and refine git commit messages.

@ChuWeiChang ChuWeiChang force-pushed the address-memory-leak-of-ifr-and-rt branch from c835321 to 0000992 Compare March 31, 2026 06:02
@ChuWeiChang ChuWeiChang requested a review from jserv March 31, 2026 06:05
jserv
jserv requested changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Improve English writing, per How to Write a Git Commit Message:

@ChuWeiChang ChuWeiChang force-pushed the address-memory-leak-of-ifr-and-rt branch from 0000992 to 0000df3 Compare March 31, 2026 06:58
@ChuWeiChang ChuWeiChang requested a review from jserv March 31, 2026 07:22
jserv
jserv requested changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

The diagnosis is correct: the hand-rolled ifr was 32 bytes, struct ifreq on 64-bit is 40 (the ifmap union member has unsigned long). Using standard headers is the right direction.

Two issues to address before merging:

  1. The PR title says "memory leaks" but this fixes a buffer overflow / stack corruption. Please retitle to match the (accurate) commit message.

  2. The blank line between system includes and project includes (before /* minislirp headers */) was intentional separation. Please keep it.

src/net-slirp.c
@@ -1037,18 +1038,7 @@ int kbox_net_configure(const struct kbox_sysnrs *sysnrs)
return -1;
Copy link
Copy Markdown
Contributor

@jserv jserv Mar 31, 2026

Choose a reason for hiding this comment

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

Every struct that crosses the lkl_syscall6 boundary in this codebase has a _Static_assert for size and critical offsets (see kbox_lkl_stat and kbox_open_how in lkl-wrap.h). This one should too:

_Static_assert(sizeof(struct ifreq) == 40,
               "struct ifreq must be 40 bytes (64-bit Linux ABI)");

src/net-slirp.c
@@ -1064,8 +1054,8 @@ int kbox_net_configure(const struct kbox_sysnrs *sysnrs)

/* 2. Set IP address via SIOCSIFADDR. */
Copy link
Copy Markdown
Contributor

@jserv jserv Mar 31, 2026

Choose a reason for hiding this comment

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

sa_data[2] is a magic offset that encodes knowledge of sockaddr_in layout without naming it. The original code had sin_family / sin_addr which were self-documenting. Cast to sockaddr_in * instead:

struct sockaddr_in *addr = (struct sockaddr_in *) &ifr.ifr_addr;
addr->sin_family = AF_INET;
inet_pton(AF_INET, GUEST_IP_STR, &addr->sin_addr);

Same for the netmask block below.

src/net-slirp.c
@@ -1093,28 +1083,7 @@ int kbox_net_configure(const struct kbox_sysnrs *sysnrs)
__atomic_store_n(&net_ready, 1, __ATOMIC_RELEASE);
Copy link
Copy Markdown
Contributor

@jserv jserv Mar 31, 2026

Choose a reason for hiding this comment

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

Same as ifreq -- add a _Static_assert for sizeof(struct rtentry). The old hand-rolled struct was missing rt_window and rt_irtt, so the size difference is the whole reason this needed fixing. Pin it so it cannot silently drift.

@ChuWeiChang ChuWeiChang changed the title Resolve memory leaks Resolve buffer overflow Mar 31, 2026
@ChuWeiChang ChuWeiChang force-pushed the address-memory-leak-of-ifr-and-rt branch from 0000df3 to 00008f0 Compare March 31, 2026 17:02
@ChuWeiChang
Fix buffer-overflow by using standard net structs
000008f 
Replace custom, hardcoded definitions of 'ifr' and 'rt'
with standard Linux headers.

The previous custom structures caused AddressSanitizer
(ASan) triggers due to memory mismatches:
    - Architecture Mismatch: The hardcoded 'ifr' struct
      was fixed at 32 bytes, whereas 64-bit architectures
      expect 40 bytes. This resulted in an 8-byte overflow
      during LKL calls.

    - Incomplete Definition: The 'rt' struct was missing
      standard fields (rt_window and rt_irtt), leading
      to incorrect memory offsets and subsequent stack corruption.

By using <net/if.h> and <net/route.h>, we ensure field alignment
across different host architectures, resolving the buffer overflow
and improving overall portability.

Resolves: sysprog21#33

Change-Id: If6e7a8ea33ba4957a87e3a30ebac04229b1896dd
@ChuWeiChang ChuWeiChang force-pushed the address-memory-leak-of-ifr-and-rt branch from 00008f0 to 000008f Compare March 31, 2026 17:24
@ChuWeiChang ChuWeiChang requested a review from jserv March 31, 2026 17:25
jserv
jserv requested changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Improve git commit messages by avoiding unintended indentations and clearly addressing the rationale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cubic-dev-ai cubic-dev-ai[bot] cubic-dev-ai[bot] left review comments

@jserv jserv jserv requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ASan stack-buffer-overflow in SLIRP networking due to struct mismatches with LKL

2 participants

@ChuWeiChang @jserv