Message-ID: <CAPZ3m_gL-K1d2r1YSZhFXmy4v3xHs5uigGOmC2TdsAAoZx+Tyg@mail.gmail.com>
Date: Thu, 17 Jul 2025 14:34:25 -0300
From: Marcelo Moreira <marcelomoreira1905@...il.com>
To: Dave Chinner <david@...morbit.com>
Cc: cem@...nel.org, linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] xfs: Replace strncpy with strscpy

Em qua., 16 de jul. de 2025 às 20:52, Dave Chinner
<david@...morbit.com> escreveu:
>
> 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@...morbit.com

got it, `kvzalloc` ensures that `sc->buf` is indeed NUL-terminated
from the start, and the explicit NUL termination (I hadn't seen that)
in `xrep_symlink_salvage()` (target_buf[buflen] = 0;) further
clarifies that the data on disk is treated as raw, non-NUL-terminated
bytes.

Thank you very much Dave for your detailed review and for taking the
time to explain the nuances of this code. I clearly misunderstood
several critical aspects of how `strncpy()` was being used here and
the protective mechanisms already in place.

My apologies for the incorrect assumptions in my commit message.

Given that the original `strncpy()` is safe and correctly implemented
for this context, and understanding that `memcpy()` would be the
correct replacement if a change were deemed necessary for
non-NUL-terminated raw data, I have a question:

Do you still think a replacement is necessary here, or would you
prefer to keep the existing `strncpy()` given its correct and safe
usage in this context? If a replacement is preferred, I will resubmit
a V2 using `memcpy()` instead.

Thank you again for teaching me these important details. I'm learning
a lot from your feedback :D

-- 
Cheers,
Marcelo Moreira

Powered by blists - more mailing lists