| Message ID | 7aad9fc3-ed65-a016-d477-5d830e31cb43@sandeen.net |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] xfs: fix string handling in get/set functions | expand |
On 06/05/2018 01:49 PM, Eric Sandeen wrote: > From: Arnd Bergmann <arnd@arndb.de> > > [sandeen: fix subject, avoid copy-out of uninit data in getlabel] > > gcc-8 reports two warnings for the newly added getlabel/setlabel code: > > fs/xfs/xfs_ioctl.c: In function 'xfs_ioc_getlabel': > fs/xfs/xfs_ioctl.c:1822:38: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess] > strncpy(label, sbp->sb_fname, sizeof(sbp->sb_fname)); > ^ > In function 'strncpy', > inlined from 'xfs_ioc_setlabel' at /git/arm-soc/fs/xfs/xfs_ioctl.c:1863:2, > inlined from 'xfs_file_ioctl' at /git/arm-soc/fs/xfs/xfs_ioctl.c:1918:10: > include/linux/string.h:254:9: error: '__builtin_strncpy' output may be truncated copying 12 bytes from a string of length 12 [-Werror=stringop-truncation] > return __builtin_strncpy(p, q, size); > > In both cases, part of the problem is that one of the strncpy() > arguments is a fixed-length character array with zero-padding rather > than a zero-terminated string. In the first one case, we also get an > odd warning about sizeof-pointer-memaccess, which doesn't seem right > (the sizeof is for an array that happens to be the same as the second > strncpy argument). > > To work around the bogus warning, I use a plain 'XFSLABEL_MAX' for > the strncpy() length when copying the label in getlabel. For setlabel(), > using memcpy() with the correct length that is already known avoids > the second warning and is slightly simpler. > > In a related issue, it appears that we accidentally skip the trailing > \0 when copying a 12-character label back to user space in getlabel(). > Using the correct sizeof() argument here copies the extra character. > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602 > Fixes: f7664b31975b ("xfs: implement online get/set fs label") > Cc: Eric Sandeen <sandeen@redhat.com> > Cc: Martin Sebor <msebor@gmail.com> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 82f7c83c1dad..596e176c19a6 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1828,13 +1828,13 @@ xfs_ioc_getlabel( > /* Paranoia */ > BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); > > + /* 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, sizeof(sbp->sb_fname)); > + strncpy(label, sbp->sb_fname, XFSLABEL_MAX); > spin_unlock(&mp->m_sb_lock); I don't see this code in my copy of the kernel so I may be missing some context but assuming label is at least one byte larger than sb_fname then the following would be how GCC expects strncpy to be used in this case: char label[sizeof sbp->sb_fname + 1]; strncpy (label, sbp->sb_fname, sizeof label - 1); label[sizeof label - 1] = '\0'; Since strncpy() zeroes out the destination past the first nul this should also obviate the call to memset() above that GCC unfortunately doesn't eliminate (I just opened bug 86061 to add this optimization). Martin > > - /* xfs on-disk label is 12 chars, be sure we send a null to user */ > - label[XFSLABEL_MAX] = '\0'; > - if (copy_to_user(user_label, label, sizeof(sbp->sb_fname))) > + if (copy_to_user(user_label, label, sizeof(label))) > return -EFAULT; > return 0; > } > @@ -1870,7 +1870,7 @@ xfs_ioc_setlabel( > > spin_lock(&mp->m_sb_lock); > memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname)); > - strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname)); > + memcpy(sbp->sb_fname, label, len); > spin_unlock(&mp->m_sb_lock); > > /* >
On Tue, Jun 05, 2018 at 02:49:20PM -0500, Eric Sandeen wrote: > From: Arnd Bergmann <arnd@arndb.de> > > [sandeen: fix subject, avoid copy-out of uninit data in getlabel] > > gcc-8 reports two warnings for the newly added getlabel/setlabel code: > > fs/xfs/xfs_ioctl.c: In function 'xfs_ioc_getlabel': > fs/xfs/xfs_ioctl.c:1822:38: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess] > strncpy(label, sbp->sb_fname, sizeof(sbp->sb_fname)); > ^ > In function 'strncpy', > inlined from 'xfs_ioc_setlabel' at /git/arm-soc/fs/xfs/xfs_ioctl.c:1863:2, > inlined from 'xfs_file_ioctl' at /git/arm-soc/fs/xfs/xfs_ioctl.c:1918:10: > include/linux/string.h:254:9: error: '__builtin_strncpy' output may be truncated copying 12 bytes from a string of length 12 [-Werror=stringop-truncation] > return __builtin_strncpy(p, q, size); > > In both cases, part of the problem is that one of the strncpy() > arguments is a fixed-length character array with zero-padding rather > than a zero-terminated string. In the first one case, we also get an > odd warning about sizeof-pointer-memaccess, which doesn't seem right > (the sizeof is for an array that happens to be the same as the second > strncpy argument). > > To work around the bogus warning, I use a plain 'XFSLABEL_MAX' for > the strncpy() length when copying the label in getlabel. For setlabel(), > using memcpy() with the correct length that is already known avoids > the second warning and is slightly simpler. > > In a related issue, it appears that we accidentally skip the trailing > \0 when copying a 12-character label back to user space in getlabel(). > Using the correct sizeof() argument here copies the extra character. > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602 > Fixes: f7664b31975b ("xfs: implement online get/set fs label") > Cc: Eric Sandeen <sandeen@redhat.com> > Cc: Martin Sebor <msebor@gmail.com> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Working around strncpy warnings with memcpy? I guess... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 82f7c83c1dad..596e176c19a6 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1828,13 +1828,13 @@ xfs_ioc_getlabel( > /* Paranoia */ > BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); > > + /* 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, sizeof(sbp->sb_fname)); > + strncpy(label, sbp->sb_fname, XFSLABEL_MAX); > spin_unlock(&mp->m_sb_lock); > > - /* xfs on-disk label is 12 chars, be sure we send a null to user */ > - label[XFSLABEL_MAX] = '\0'; > - if (copy_to_user(user_label, label, sizeof(sbp->sb_fname))) > + if (copy_to_user(user_label, label, sizeof(label))) > return -EFAULT; > return 0; > } > @@ -1870,7 +1870,7 @@ xfs_ioc_setlabel( > > spin_lock(&mp->m_sb_lock); > memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname)); > - strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname)); > + memcpy(sbp->sb_fname, label, len); > spin_unlock(&mp->m_sb_lock); > > /* > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 82f7c83c1dad..596e176c19a6 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1828,13 +1828,13 @@ xfs_ioc_getlabel( > /* Paranoia */ > BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); > > + /* 1 larger than sb_fname, so this ensures a trailing NUL char */ > + memset(label, 0, sizeof(label)); I don't get the comment. In fact I don't even get why we need any comment here. This is a structure that gets copied to userspace, and we zero the whole structure, as we should do by default for anything that goes to userspace. Otherwise this looks fine to me.
On 6/6/18 5:58 AM, Christoph Hellwig wrote: >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index 82f7c83c1dad..596e176c19a6 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -1828,13 +1828,13 @@ xfs_ioc_getlabel( >> /* Paranoia */ >> BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); >> >> + /* 1 larger than sb_fname, so this ensures a trailing NUL char */ >> + memset(label, 0, sizeof(label)); > > I don't get the comment. In fact I don't even get why we need any > comment here. This is a structure that gets copied to userspace, > and we zero the whole structure, as we should do by default for > anything that goes to userspace. Sure, I guess we didn't really need the comment, my main point was that we were guaranteed to have a \0 remaining at the end because we'd never copy over it due to sb_fname's smaller size. > Otherwise this looks fine to me. Thanks. In retrospect we could have gone back to Arnd's original patch but I guess the explicit memset is ... well, nice and explicit. -Eric
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 82f7c83c1dad..596e176c19a6 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1828,13 +1828,13 @@ xfs_ioc_getlabel( /* Paranoia */ BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); + /* 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, sizeof(sbp->sb_fname)); + strncpy(label, sbp->sb_fname, XFSLABEL_MAX); spin_unlock(&mp->m_sb_lock); - /* xfs on-disk label is 12 chars, be sure we send a null to user */ - label[XFSLABEL_MAX] = '\0'; - if (copy_to_user(user_label, label, sizeof(sbp->sb_fname))) + if (copy_to_user(user_label, label, sizeof(label))) return -EFAULT; return 0; } @@ -1870,7 +1870,7 @@ xfs_ioc_setlabel( spin_lock(&mp->m_sb_lock); memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname)); - strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname)); + memcpy(sbp->sb_fname, label, len); spin_unlock(&mp->m_sb_lock); /*