* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-05-25 15:14 [PATCH] xfs: mark sb_fname as nonstring Arnd Bergmann
@ 2018-05-25 16:52 ` Christoph Hellwig
2018-05-25 20:18 ` Arnd Bergmann
2018-05-25 16:53 ` Eric Sandeen
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2018-05-25 16:52 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Darrick J. Wong, linux-xfs, Eric Sandeen, Martin Sebor,
Brian Foster, Dave Chinner, Eric Sandeen, Dan Williams,
Ross Zwisler, linux-kernel
> 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?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-05-25 16:52 ` Christoph Hellwig
@ 2018-05-25 20:18 ` Arnd Bergmann
0 siblings, 0 replies; 14+ messages in thread
From: Arnd Bergmann @ 2018-05-25 20:18 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Darrick J. Wong, linux-xfs, Eric Sandeen, Martin Sebor,
Brian Foster, Dave Chinner, Eric Sandeen, Dan Williams,
Ross Zwisler, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-05-25 15:14 [PATCH] xfs: mark sb_fname as nonstring Arnd Bergmann
2018-05-25 16:52 ` Christoph Hellwig
@ 2018-05-25 16:53 ` Eric Sandeen
2018-05-25 20:16 ` Arnd Bergmann
2018-06-05 18:44 ` Eric Sandeen
2018-06-05 19:49 ` [PATCH V2] xfs: fix string handling in get/set functions Eric Sandeen
3 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2018-05-25 16:53 UTC (permalink / raw)
To: Arnd Bergmann, Darrick J. Wong, linux-xfs
Cc: Martin Sebor, Brian Foster, Dave Chinner, Eric Sandeen,
Dan Williams, Ross Zwisler, linux-kernel
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);
>
> /*
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-05-25 16:53 ` Eric Sandeen
@ 2018-05-25 20:16 ` Arnd Bergmann
2018-05-25 20:21 ` Eric Sandeen
0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2018-05-25 20:16 UTC (permalink / raw)
To: Eric Sandeen
Cc: Darrick J. Wong, linux-xfs, Martin Sebor, Brian Foster,
Dave Chinner, Eric Sandeen, Dan Williams, Ross Zwisler,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-05-25 20:16 ` Arnd Bergmann
@ 2018-05-25 20:21 ` Eric Sandeen
0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2018-05-25 20:21 UTC (permalink / raw)
To: Arnd Bergmann, Eric Sandeen
Cc: Darrick J. Wong, linux-xfs, Martin Sebor, Brian Foster,
Dave Chinner, Dan Williams, Ross Zwisler,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-05-25 15:14 [PATCH] xfs: mark sb_fname as nonstring Arnd Bergmann
2018-05-25 16:52 ` Christoph Hellwig
2018-05-25 16:53 ` Eric Sandeen
@ 2018-06-05 18:44 ` Eric Sandeen
2018-06-05 21:23 ` Arnd Bergmann
2018-06-05 19:49 ` [PATCH V2] xfs: fix string handling in get/set functions Eric Sandeen
3 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2018-06-05 18:44 UTC (permalink / raw)
To: Arnd Bergmann, Darrick J. Wong, linux-xfs
Cc: Eric Sandeen, Martin Sebor, Brian Foster, Dave Chinner,
Dan Williams, Ross Zwisler, linux-kernel
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);
>
> /*
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-06-05 18:44 ` Eric Sandeen
@ 2018-06-05 21:23 ` Arnd Bergmann
2018-06-05 21:51 ` Eric Sandeen
0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2018-06-05 21:23 UTC (permalink / raw)
To: Eric Sandeen
Cc: Darrick J. Wong, linux-xfs, Eric Sandeen, Martin Sebor,
Brian Foster, Dave Chinner, Dan Williams, Ross Zwisler,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] xfs: mark sb_fname as nonstring
2018-06-05 21:23 ` Arnd Bergmann
@ 2018-06-05 21:51 ` Eric Sandeen
0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2018-06-05 21:51 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Darrick J. Wong, linux-xfs, Eric Sandeen, Martin Sebor,
Brian Foster, Dave Chinner, Dan Williams, Ross Zwisler,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2] xfs: fix string handling in get/set functions
2018-05-25 15:14 [PATCH] xfs: mark sb_fname as nonstring Arnd Bergmann
` (2 preceding siblings ...)
2018-06-05 18:44 ` Eric Sandeen
@ 2018-06-05 19:49 ` Eric Sandeen
2018-06-05 21:28 ` Martin Sebor
` (2 more replies)
3 siblings, 3 replies; 14+ messages in thread
From: Eric Sandeen @ 2018-06-05 19:49 UTC (permalink / raw)
To: Arnd Bergmann, Darrick J. Wong, linux-xfs
Cc: Eric Sandeen, Martin Sebor, Brian Foster, Dave Chinner,
Dan Williams, Ross Zwisler, linux-kernel
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);
- /* 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);
/*
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH V2] xfs: fix string handling in get/set functions
2018-06-05 19:49 ` [PATCH V2] xfs: fix string handling in get/set functions Eric Sandeen
@ 2018-06-05 21:28 ` Martin Sebor
2018-06-06 2:59 ` Darrick J. Wong
2018-06-06 10:58 ` Christoph Hellwig
2 siblings, 0 replies; 14+ messages in thread
From: Martin Sebor @ 2018-06-05 21:28 UTC (permalink / raw)
To: Eric Sandeen, Arnd Bergmann, Darrick J. Wong, linux-xfs
Cc: Eric Sandeen, Brian Foster, Dave Chinner, Dan Williams,
Ross Zwisler, linux-kernel
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);
>
> /*
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH V2] xfs: fix string handling in get/set functions
2018-06-05 19:49 ` [PATCH V2] xfs: fix string handling in get/set functions Eric Sandeen
2018-06-05 21:28 ` Martin Sebor
@ 2018-06-06 2:59 ` Darrick J. Wong
2018-06-06 10:58 ` Christoph Hellwig
2 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2018-06-06 2:59 UTC (permalink / raw)
To: Eric Sandeen
Cc: Arnd Bergmann, linux-xfs, Eric Sandeen, Martin Sebor,
Brian Foster, Dave Chinner, Dan Williams, Ross Zwisler,
linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH V2] xfs: fix string handling in get/set functions
2018-06-05 19:49 ` [PATCH V2] xfs: fix string handling in get/set functions Eric Sandeen
2018-06-05 21:28 ` Martin Sebor
2018-06-06 2:59 ` Darrick J. Wong
@ 2018-06-06 10:58 ` Christoph Hellwig
2018-06-06 17:45 ` Eric Sandeen
2 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2018-06-06 10:58 UTC (permalink / raw)
To: Eric Sandeen
Cc: Arnd Bergmann, Darrick J. Wong, linux-xfs, Eric Sandeen,
Martin Sebor, Brian Foster, Dave Chinner, Dan Williams,
Ross Zwisler, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] xfs: fix string handling in get/set functions
2018-06-06 10:58 ` Christoph Hellwig
@ 2018-06-06 17:45 ` Eric Sandeen
0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2018-06-06 17:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Arnd Bergmann, Darrick J. Wong, linux-xfs, Eric Sandeen,
Martin Sebor, Brian Foster, Dave Chinner, Dan Williams,
Ross Zwisler, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread