Re: [PATCH 1/1] xfs: online repair of symbolic links

[Christoph Hellwig <hch@infradead.org>]

On Wed, Feb 28, 2024 at 03:46:30PM -0800, Darrick J. Wong wrote:
> If scrub (or the regular verifiers) hit anything, then we end up in
> symlink_repair.c with CORRUPT set.  In this case we set the target to
> DUMMY_TARGET.

Yes.

> If the salvage functions recover fewer bytes than i_disk_size, then
> we'll set the target to DUMMY_TARGET because that could lead to things
> like:
> 
> 0. touch autoexec autoexec@bat
> 1. ln -s 'autoexec@bat' victimlink
> 2. corrupt victimlink by s/@/\0/g' on the target
> 3. repair salvages the target and ends up with 'autoexec'
> 
> Alternately:
> 
> 0. touch autoexec autoexec@bat
> 1. ln -s 'autoexec@bat' victimlink
> 2. corrupt victimlink by incrementing di_size (it's now 13)
> 3. repair salvages the target and ends up with "autoexec@bat\0"
> 
> In both of those cases, something's inconsistent between the buffer
> contents and di_size.

Yes.

> There aren't supposed to be nulls in the target,
> but whatever might have been in that byte originally is long gone.  The
> only thing to do here is replace it with DUMMY_TARGET.
> 
> If salvage recovers more bytes than i_disk_size then we have no idea if
> di_size was broken or not because the target isn't null-terminated.
> In theory the kernel will never do this (because it zeroes the xfs_buf
> contents in xfs_trans_buf_get) but fuzzers could do that.

Now why do we even want to salvage parts of the symlink?  A truncated
symlink generally would cause more harm than just refusing to follow it.