Skip to content

Conversation

@w4123
Copy link

@w4123 w4123 commented Jan 16, 2025

Capability bound error can occur when using ext2fs. This is caused by

entp = (struct ext2fs_htree_entry *)(((char *)&rootp->h_info) + rootp->h_info.h_info_len);

which limits the bound to the h_info subobject, but the entp pointer is actually used to access other subobjects in the structure.
This PR fixes the issue by preserving the bound.

@jrtc27
Copy link
Member

jrtc27 commented Jan 16, 2025

Please read the FreeBSD style guide if you're going to be contributing patches: https://man.freebsd.org/style(9)

Also, this seems dubious. Why isn't it just offsetting from h_entries?

@w4123
Copy link
Author

w4123 commented Jan 16, 2025

Please read the FreeBSD style guide if you're going to be contributing patches: https://man.freebsd.org/style(9)

Also, this seems dubious. Why isn't it just offsetting from h_entries?

I am not exactly sure. I guess it might be to maximize compatibility in case h_info_len is different in the current filesystem. But I am not aware of any case where h_info_len is not 8.

@jrtc27
Copy link
Member

jrtc27 commented Jan 16, 2025

It's code that came out of a GSoC project (see 91f5a46) so it does not surprise me that it's doing wacky, unnecessary things.

@w4123 w4123 force-pushed the fix_ext2fs_capability branch from b8752ed to c7bc78b Compare January 16, 2025 19:23
@jrtc27
Copy link
Member

jrtc27 commented Jan 16, 2025

Hm, so Linux does do the same: https://github.com/torvalds/linux/blob/619f0b6fad524f08d493a98d55bac9ab8895e3a6/fs/ext4/namei.c#L886-L887

But it also has a check in one location that it matches the expected value.

@jrtc27
Copy link
Member

jrtc27 commented Jan 16, 2025

So, if it were me, I'd just verify, along with the various other checks, that h_info_len matches the size of the struct, and then set entp to h_entries.

(I imagine a malicious disk can easily exploit this otherwise on a non-CHERI system)


entp = (struct ext2fs_htree_entry *)(((char *)&rootp->h_info) +
/* Preserve capability bound here. */
entp = (struct ext2fs_htree_entry *)(((char *)rootp) +
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we want to use __unbounded_addressof here from sys/cdefs.h? This is what we generally have been using for sub-object bounds related things.

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