Re: [PATCH] xfs: Replace strncpy with strscpy

[Dave Chinner <david@fromorbit.com>]

On Wed, Jul 16, 2025 at 03:20:37PM -0300, Marcelo Moreira wrote:
> The `strncpy` function is deprecated for NUL-terminated strings as
> explained in the "strncpy() on NUL-terminated strings" section of
> Documentation/process/deprecated.rst.
> 
> In `xrep_symlink_salvage_inline()`, the `target_buf` (which is `sc->buf`)
> is intended to hold a NUL-terminated symlink path. The original code
> used `strncpy(target_buf, ifp->if_data, nr)`, where `nr` is the maximum
> number of bytes to copy.

Yes, this prevents source buffer overrun in the event the corrupted
symlink data buffer is not correctly nul terminated or there is a
length mismatch between the symlink data and the inode metadata.

This patch removes the explicit source buffer overrun protection the
code currently provides.

> This approach is problematic because `strncpy()`
> does not guarantee NUL-termination if the source string is truncated
> exactly at `nr` bytes, which can lead to out-of-bounds read issues
> if the buffer is later treated as a NUL-terminated string.

No, that can't happen, because sc->buf is initialised to contain
NULs and is large enough to hold a max length symlink plus the
trailing NUL termination.  Hence it will always be NUL-terminated,
even if the symlink length (nr) is capped at XFS_SYMLINK_MAXLEN.

> Evidence from `fs/xfs/scrub/symlink.c` (e.g., `strnlen(sc->buf,
> XFS_SYMLINK_MAXLEN)`) confirms that `sc->buf` is indeed expected to be
> NUL-terminated.

Please read the code more carefully. This is -explicitly- called out
in a comment in xrep_symlink_salvage() before it starts to process
the symlink data buffer that we just used strncpy() to place the
data in:

		/*
                 * NULL-terminate the buffer because the ondisk target does not
                 * do that for us.  If salvage didn't find the exact amount of
                 * data that we expected to find, don't salvage anything.
                 */
                target_buf[buflen] = 0;
                if (strlen(target_buf) != sc->ip->i_disk_size)
                        buflen = 0;

Also, have a look at the remote symlink data copy above the inline
salvage code you are changing (xrep_symlink_salvage_remote()).

It uses memcpy() to reconstruct the symlink data from multiple
source buffers. It does *not* explicitly NUL-terminate sc->buf after
using memcpy() to copy from the on disk structures to sc->buf. The
on-disk symlink data is *not* NUL-terminated, either.

IOWs, the salvage code that reconstructs the symlink data does not
guarantee NUL termination, so we do it explicitly before the data in
the returned buffer is used.

> Furthermore, `sc->buf` is allocated with
> `kvzalloc(XFS_SYMLINK_MAXLEN + 1, ...)`, explicitly reserving space for
> the NUL terminator.

.... and initialising the entire buffer to contain NULs.  IOWs, we
have multiple layers of defence against data extraction not doing
NUL-termination of the data it extracts.

> This change improves code safety and clarity by using a safer function for
> string copying.

I disagree. It is a bad change because it uses strscpy() incorrectly
for the context. i.e. it removes explicit source buffer overrun
protection whilst making the incorrect assumption that the callers
need to be protected from unterminated strings in the destination
buffer.

This code is dealing with *corrupt structures*, so it -must not-
make any assumptions about the validity of incoming data structures,
nor the validity of recovered data.  It has to treat them as is they
are always invalid, and explicitly protect against all types of
buffer overruns.

IOW, if you must replace strncpy() in xrep_symlink_salvage_inline(),
then the correct replacement memcpy().  Using some other strcpy()
variant is just as easy to get wrong as strncpy() if you don't
understand why strncpy() is safe to use in the first place.

-Dave.
-- 
Dave Chinner
david@fromorbit.com