Re: [PATCH] fs/xfs: replace strncpy with strscpy

[Pranav Tyagi <pranav.tyagi03@gmail.com>]

On Mon, Jun 30, 2025 at 2:09 PM Carlos Maiolino <cem@kernel.org> wrote:
>
> On Tue, Jun 17, 2025 at 06:15:46PM +0530, Pranav Tyagi wrote:
> > Replace the deprecated strncpy() with strscpy() as the destination
> > buffer should be NUL-terminated and does not require any trailing
> > NUL-padding. Also, since NUL-termination is guaranteed,
>
> NUL-termination is only guaranteed if you copy into the buffer one less
> byte than the label requires, i.e XFSLABEL_MAX.
>
> > use sizeof(label) in place of XFSLABEL_MAX as the size
> > parameter.
>
> This is wrong, see below why.
>
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > ---
> >  fs/xfs/xfs_ioctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index d250f7f74e3b..9f4d68c5b5ab 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -992,7 +992,7 @@ xfs_ioc_getlabel(
> >       /* 1 larger than sb_fname, so this ensures a trailing NUL char */
> >       memset(label, 0, sizeof(label));
> >       spin_lock(&mp->m_sb_lock);
> > -     strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> > +     strscpy(label, sbp->sb_fname, sizeof(label));
>
> This is broken and you created a buffer overrun here.
>
> XFSLABEL_MAX is set to 12 bytes. The current label size is 13 bytes:
>
> char                    label[XFSLABEL_MAX + 1];
>
> This ensures the label will always have a null termination character as
> long as you copy XFSLABEL_MAX bytes into the label.
>
> - strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
>
> Copies 12 bytes from sb_fname into label. This ensures we always have a
> trailing \0 at the last byte.
>
> Your version:
>
> strscpy(label, sbp->sb_fname, sizeof(label));
>
> Copies 13 bytes from sb_fname into the label buffer.
>
> This not only could have copied a non-null byte to the last byte in the
> label buffer, but also But sbp->sb_fname size is XFSLABEL_MAX, so you
> are reading beyond the source buffer size, causing a buffer overrun as you
> can see on the kernel test robot report.
>
> Carlos
>
> >       spin_unlock(&mp->m_sb_lock);
> >
> >       if (copy_to_user(user_label, label, sizeof(label)))
> > --
> > 2.49.0
> >

Hi,

Thank you for the feedback. I understand that my patch is incorrect and
it causes a buffer overrun. The destination buffer is indeed, already, null
terminated. Would you like me to send a corrected patch which uses
strscpy() (as strncpy() is deprecated)?

Regret the inconvenience.

Regards
Pranav Tyagi