On Mon, Jun 30, 2025 at 2:09 PM Carlos Maiolino <cem@xxxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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
