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
