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

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 29, 2024 at 05:25:01AM -0800, Christoph Hellwig wrote:
> 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.

We don't want to salvage in that case.  I forgot to finish that last
paragraph:

"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.  Set the
target to DUMMY_TARGET in this case."

and maybe add:

"The symlink target will be preserved if scrub does not find any errors
in the symlink file, the number of bytes recovered matches i_disk_size,
and there are no nulls in the recovered target.  In all other cases it
is set to DUMMY_TARGET."

--D