On Mon, Jun 30, 2025 at 02:36:01PM +0530, Pranav Tyagi wrote: > 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)? Sure, do so. Carlos > > Regret the inconvenience. > > Regards > Pranav Tyagi
