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.

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

Out of Bounds Memory Access with small btree node sizes #9

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@modelrockettier
Contributor

modelrockettier commented Nov 3, 2018

Running bcachefs-tools with GCC's address sanitizer enabled alerts to an out of bounds heap access when checking a filesystem with a small btree node size (under 4KB).

To reproduce:

  • Build bcachefs-tools with GCC's address sanitizer
    make EXTRA_CFLAGS=-fsanitize=address LOADLIBES=-lasan
  • Create a small disk image
    truncate -s 4M disk.img
  • Format the disk image
    ./bcachefs format -f --btree_node=2048 disk.img
  • Run fsck twice (the first time to initialize the filesystem, the second time will trigger the bug)
    ./bcachefs fsck disk.img
    ./bcachefs fsck disk.img

Note: on Ubuntu 18.04, I had to preload libasan for it to work, ymmv in this regard. Just prepend the following to all the above bcachefs commands.
LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libasan.so.4

The relevant call stack from the bug:

==63435==ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 4
# 0 0x56286c4924f0 in bch2_bio_map libbcachefs/util.c:536
# 1 0x56286c3e4c06 in bch2_btree_node_read libbcachefs/btree_io.c:1440
# 2 0x56286c3e51e4 in bch2_btree_root_read libbcachefs/btree_io.c:1490
# 3 0x56286c421344 in bch2_fs_recovery libbcachefs/recovery.c:195
# 4 0x56286c45f7d5 in bch2_fs_start libbcachefs/super.c:713
# 5 0x56286c463760 in bch2_fs_open libbcachefs/super.c:1698
# 6 0x56286c3542ad in cmd_fsck cmd_fsck.c:59
# 7 0x56286c34b295 in main bcachefs.c:158

The issue appears to stem from a btree node that spans 2 pages, but since the node size is < 1 page, bio_alloc_bioset() is currently only allocating 1 inline bio_vec struct to hold the bio data (when it needs 2).

In theory this would affect any btree nodes that aren't page aligned, but I've only been able to produce the bug with btree node sizes under 4KB.

@koverstreet

This comment has been minimized.

Owner

koverstreet commented Nov 4, 2018

Thanks for the bug report!

I fixed it a bit differently - we don't really want to allocate an extra bvec when it's not needed, since usually (at least in the kernel) the btree node buffer will be page aligned, and it'll be a power of two size, so allocating an extra bvec will often end up bumping up the size of the allocation.

There were also a couple other bio allocations that needed to be fixed - but, good catch :)

@koverstreet koverstreet closed this Nov 4, 2018

@koverstreet

This comment has been minimized.

Owner

koverstreet commented on bd55190 Nov 4, 2018

Whoops, I was looking at your other request - I applied your change manually before I saw that you had one against the correct repository that I could pull from. Anyways, thanks for the fix :)

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