| Message ID | 20180525151421.2317292-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: mark sb_fname as nonstring | expand |
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 84fbf164cbc3..eb79f2bc4dcc 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( > BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); > > 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); Hmm, shouldn't we just do a memcpy here? Also given that the kernel never even looks at sb_fname maybe we can turn into an array of unsigned chars to escape those string warnings?
On 5/25/18 10:14 AM, Arnd Bergmann wrote: > gcc-8 reports two warnings for the newly added getlabel/setlabel code: Thanks for catching these. The patch summary confuses me, what does "mark sb_fname as nonstring" mean in the context of the actual patch? I hate strings in C! ;) > 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)); ^ o_O hrpmh. > 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. Oops, you are correct. Sigh, I wonder why testing didn't see that. > 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> > --- > fs/xfs/xfs_ioctl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 84fbf164cbc3..eb79f2bc4dcc 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( > BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); > > spin_lock(&mp->m_sb_lock); > - strncpy(label, sbp->sb_fname, sizeof(sbp->sb_fname)); > + strncpy(label, sbp->sb_fname, XFSLABEL_MAX); ok > 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))) ok. (odd how this is ok for copy_to_user but not for strncpy above) :) > return -EFAULT; > return 0; > } > @@ -1860,7 +1860,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); Hm but len = strnlen(label, XFSLABEL_MAX + 1); which could be one longer than sbp->sb_fname, no? > spin_unlock(&mp->m_sb_lock); > > /* >
On Fri, May 25, 2018 at 6:53 PM, Eric Sandeen <sandeen@redhat.com> wrote: > On 5/25/18 10:14 AM, Arnd Bergmann wrote: >> >> gcc-8 reports two warnings for the newly added getlabel/setlabel code: > > > Thanks for catching these. > > The patch summary confuses me, what does "mark sb_fname as nonstring" > mean in the context of the actual patch? My mistake. I tried a few different approaches and ended up using the subject line from an earlier version with a later patch. The 'nonstring' annotation is a variable attribute that gets gcc-8 to shut up about -Wstringop-truncation warnings, and is intended to mark those character arrays that are not expected to be null-terminated but still used with strncpy(). >> 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))) > > > > ok. (odd how this is ok for copy_to_user but not for strncpy above) :) No idea. Maybe the gcc bug only happens with struct members but not local variables? >> return -EFAULT; >> return 0; >> } >> @@ -1860,7 +1860,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); > > > Hm but len = strnlen(label, XFSLABEL_MAX + 1); > which could be one longer than sbp->sb_fname, no? We have an explicit check for that, so I think it's ok: if (len > sizeof(sbp->sb_fname)) return -EINVAL; Arnd
On Fri, May 25, 2018 at 6:52 PM, Christoph Hellwig <hch@infradead.org> wrote: >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index 84fbf164cbc3..eb79f2bc4dcc 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( >> BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); >> >> 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); > > Hmm, shouldn't we just do a memcpy here? I thought about that as well, but decided that strncpy()'s zero-padding is better here than padding with potentially random contents of the user space stack. > Also given that the kernel never even looks at sb_fname maybe > we can turn into an array of unsigned chars to escape those string > warnings? I don't think that makes a difference to gcc. Arnd
On 5/25/18 3:16 PM, Arnd Bergmann wrote: > On Fri, May 25, 2018 at 6:53 PM, Eric Sandeen <sandeen@redhat.com> wrote: >> On 5/25/18 10:14 AM, Arnd Bergmann wrote: ... >>> @@ -1860,7 +1860,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); >> >> >> Hm but len = strnlen(label, XFSLABEL_MAX + 1); >> which could be one longer than sbp->sb_fname, no? > > We have an explicit check for that, so I think it's ok: > > if (len > sizeof(sbp->sb_fname)) > return -EINVAL; Ah so we do; I wrote this at least 2 weeks ago, you're asking a lot for me to remember it. (or to even read it, apparently). ;) Thanks, -Eric
On 5/25/18 10:14 AM, Arnd Bergmann wrote: > 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> > --- > fs/xfs/xfs_ioctl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 84fbf164cbc3..eb79f2bc4dcc 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( > BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); > > 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))) I /think/ this also runs the risk of copying out stack memory. I'll send another proposal based on this with slight modifications. > return -EFAULT; > return 0; > } > @@ -1860,7 +1860,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 5, 2018 at 8:44 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > On 5/25/18 10:14 AM, Arnd Bergmann wrote: >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index 84fbf164cbc3..eb79f2bc4dcc 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( >> BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); >> >> 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))) > > I /think/ this also runs the risk of copying out stack memory. > > I'll send another proposal based on this with slight modifications. I assumed it's safe since the earlier strncpy() pads the local 'label' with zero bytes up the XFSLABEL_MAX, and the last byte is explicitly set to guarantee zero padding. Using strlcpy() or strscpy() would guarantee a zero-terminated string without the explicit ='\0' assignment but would risk the data leak you were probably thinking of. Arnd
On 6/5/18 4:23 PM, Arnd Bergmann wrote: > On Tue, Jun 5, 2018 at 8:44 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >> On 5/25/18 10:14 AM, Arnd Bergmann wrote: > >>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >>> index 84fbf164cbc3..eb79f2bc4dcc 100644 >>> --- a/fs/xfs/xfs_ioctl.c >>> +++ b/fs/xfs/xfs_ioctl.c >>> @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( >>> BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); >>> >>> 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))) >> >> I /think/ this also runs the risk of copying out stack memory. >> >> I'll send another proposal based on this with slight modifications. > > I assumed it's safe since the earlier strncpy() pads the local 'label' > with zero bytes up the XFSLABEL_MAX, and the last byte > is explicitly set to guarantee zero padding. Ah you are right, I forgot that strncpy pads. Did I mention that I hate C strings... o_O In that case I suppose your original patch is fine, if you'd like to fix the "mark as nonstring" subject. Thanks, -Eric
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 84fbf164cbc3..eb79f2bc4dcc 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1819,12 +1819,12 @@ xfs_ioc_getlabel( BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX); 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; } @@ -1860,7 +1860,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); /*
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> --- fs/xfs/xfs_ioctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.9.0