Skip to content

libblkid: (bcachefs) fix not detecting large superblocks#2427

Merged
karelzak merged 1 commit intoutil-linux:masterfrom
breavyn:master
Aug 23, 2023
Merged

libblkid: (bcachefs) fix not detecting large superblocks#2427
karelzak merged 1 commit intoutil-linux:masterfrom
breavyn:master

Conversation

@breavyn
Copy link
Copy Markdown
Contributor

@breavyn breavyn commented Aug 9, 2023

The bcachefs probe has a maximum superblock size hardcoded to 4KiB. This is causing it to miss filesystems whose superblock has grown larger than 4KiB. The maximum potential size of the superblock is encoded at a fixed location within the superblock, so this value should be used instead.

Copy link
Copy Markdown
Member

@t-8ch t-8ch left a comment

Choose a reason for hiding this comment

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

This is a nice validation.

Another goal this hardcoded limit achieves is to limit the amount of data that can be read from a malicious disk image. We would loose this goal in the current state.

@breavyn
Copy link
Copy Markdown
Contributor Author

breavyn commented Aug 9, 2023

The default maximum size is currently 1MiB, but can be set higher. I'm not sure how big they could realistically get. We could have both this check, and a hard limit at 10MiB or so. I'll get in touch with the bcachefs devs and try to find what an appropriate limit is.

@t-8ch
Copy link
Copy Markdown
Member

t-8ch commented Aug 9, 2023

Thanks!

Thinking about this some more:
@karelzak
Maybe blkid_probe_get_sb_buffer() itself should already enforce some limits to avoid DoS issues.

Probing does not detect bcachefs filesystems with a superblock larger
than 4KiB. Bcachefs superblocks grow in size and can become much larger
than this.

Increase the superblock maximum size limit to 1MiB.

Validate the superblock isn't larger than the maximum size defined in
the superblocks layout section.
@breavyn
Copy link
Copy Markdown
Contributor Author

breavyn commented Aug 10, 2023

I've received advice that 1MiB is fine for the hard limit and restored the check.

Copy link
Copy Markdown
Member

@t-8ch t-8ch left a comment

Choose a reason for hiding this comment

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

Thanks!

@karelzak
Copy link
Copy Markdown
Collaborator

Thanks!

Thinking about this some more: @karelzak Maybe blkid_probe_get_sb_buffer() itself should already enforce some limits to avoid DoS issues.

Good question.

Now libblkid/src/probe.c: read_buffer() uses ULONG_MAX as the limit, and blkid_probe_get_buffer() checks if the requested area fits into the device. There is no other limit.

The question is, what is the right generic limit?

IMHO, the ideal is to check for proper limits in the probing functions where we call blkid_probe_get_buffer() and blkid_probe_get_sb_buffer(). The generic limit we can set to something crazy (10MiB? ... not sure)

@t-8ch
Copy link
Copy Markdown
Member

t-8ch commented Aug 15, 2023

Maybe 8MiB, it's a nice power-of-two.

t-8ch added a commit to t-8ch/util-linux that referenced this pull request Aug 15, 2023
Many probers read data from disk bounded by some field from the probed
disk itself.
The probers should validate the read length before using.
Add a fallback that kicks in when the proper does not check the length
epxlicitly.

See util-linux#2427
Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
@karelzak karelzak merged commit 4c2aea4 into util-linux:master Aug 23, 2023
karelzak added a commit that referenced this pull request Aug 23, 2023
Addresses: #2427
Signed-off-by: Karel Zak <kzak@redhat.com>
t-8ch pushed a commit to t-8ch/util-linux that referenced this pull request Sep 22, 2023
Addresses: util-linux#2427
Signed-off-by: Karel Zak <kzak@redhat.com>
(cherry picked from commit 17873d3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants