Page MenuHomeFreeBSD

ufs: Rework shortlink handling to avoid subobject overflows
ClosedPublic

Authored by jrtc27 on Dec 24 2021, 4:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 16 2024, 4:09 PM
Unknown Object (File)
Mar 11 2024, 1:49 PM
Unknown Object (File)
Mar 11 2024, 1:49 PM
Unknown Object (File)
Mar 11 2024, 1:49 PM
Unknown Object (File)
Mar 11 2024, 1:49 PM
Unknown Object (File)
Mar 8 2024, 2:00 AM
Unknown Object (File)
Feb 11 2024, 11:33 PM
Unknown Object (File)
Jan 12 2024, 8:33 AM
Subscribers

Details

Summary

Shortlinks occupy the space of both di_db and di_ib when used. However,
everywhere that wants to read or write a shortlink takes a pointer do
di_db and promptly runs off the end of it into di_ib. This is fine on
most architectures, if a little dodgy. However, on CHERI, the compiler
can optionally restrict the bounds on pointers to subobjects to just
that subobject, in order to mitigate intra-object buffer overflows, and
this is enabled in CheriBSD's pure-capability kernels.

Instead, clean this up by inserting a union such that a new di_shortlink
can be added with the right size and element type, avoiding the need to
cast and allowing the use of the DIP macro to access the field. This
also mirrors how the ext2fs code implements extents support, with the
exact same structure other than having a uint32_t i_data[] instead of a
char di_shortlink[].

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb added a subscriber: jhb.

I think this is fine, but probably best to let Kirk sign off as well.

This revision is now accepted and ready to land.Dec 24 2021, 4:51 PM

The idea and implementation look good. I would like my noted style comments handled.

The changes to usr.sbin/makefs/ffs/ufs_inode.h made me wonder how i_ffs1_shortlink and i_ffs2_shortlink ever compiled. The answer is because they are never used in the code.

sys/ufs/ufs/dinode.h
151

Please shorten this comment to make it fit the line. I suggest changing "blocks" to "blks" and if also necessary drop space between ; and /*.

153

add comment /* 112: Embedded symbolic link. */ on this line aligned with comments that follow it (i.e., di_modrev).

193

Please shorten this comment to make it fit the line. I suggest changing "blocks" to "blks" and if also necessary drop space between ; and /*.

195

add comment /* 40: Embedded symbolic link. */ on this line aligned with comments that follow it (i.e., di_flags).

I struggled to find a good way to format those unions so having you tell me what to do there is appreciated :)

And yes, I had the same thought re i_ffsX_shortlink when I stumbled upon them (by chance, looking for any instance of shortlink)

  • Added di_shortlink comments
  • Made di_db/di_ib comments fit in 80 chars

Even shortening "blocks" to "blks" and removing the leading space the lines were still too long. I could only get them to fit all one one line as "Dir/Ind blks.", which seemed a bit too terse; I can switch to that if you'd rather, but I've gone with wrapping the array length like for di_shortlink instead in this version.

This revision now requires review to proceed.Jan 2 2022, 9:35 AM

Your comment wrap rework looks good. As you note, it is consistent with the way di_shortlink is commented.

This revision is now accepted and ready to land.Jan 2 2022, 8:44 PM