| Message ID | 20240405-strncpy-xattr-split2-v1-1-90ab18232407@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: xattr: replace strncpy and check for truncation | expand |
On Fri, Apr 05, 2024 at 07:45:08PM +0000, Justin Stitt wrote: > - memcpy(offset, prefix, prefix_len); > - offset += prefix_len; > - strncpy(offset, (char *)name, namelen); /* real name */ > - offset += namelen; > - *offset = '\0'; > + > + combined_len = prefix_len + namelen; > + > + /* plus one byte for \0 */ > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); > + > + if (actual_len < combined_len) Shouldn't this be a != ? That being said I think this is actually wrong - the attr names are not NULL-terminated on disk, which is why we have the explicit zero terminataion above. How was this tested?
Hi, On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Fri, Apr 05, 2024 at 07:45:08PM +0000, Justin Stitt wrote: > > - memcpy(offset, prefix, prefix_len); > > - offset += prefix_len; > > - strncpy(offset, (char *)name, namelen); /* real name */ > > - offset += namelen; > > - *offset = '\0'; > > + > > + combined_len = prefix_len + namelen; > > + > > + /* plus one byte for \0 */ > > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); > > + > > + if (actual_len < combined_len) > > Shouldn't this be a != ? I guess it could be. It's a truncation check so I figured just checking if the amount of bytes actually copied was less than the total would suffice. > > That being said I think this is actually wrong - the attr names are > not NULL-terminated on disk, which is why we have the explicit > zero terminataion above. Gotcha, in which case we could use the "%.*s" format specifier which allows for a length argument. Does something like this look better? diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index 364104e1b38a..1b7e886e0f29 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -206,6 +206,7 @@ __xfs_xattr_put_listent( { char *offset; int arraytop; + size_t combined_len, actual_len; if (context->count < 0 || context->seen_enough) return; @@ -220,11 +221,16 @@ __xfs_xattr_put_listent( return; } offset = context->buffer + context->count; - memcpy(offset, prefix, prefix_len); - offset += prefix_len; - strncpy(offset, (char *)name, namelen); /* real name */ - offset += namelen; - *offset = '\0'; + + combined_len = prefix_len + namelen; + + /* plus one byte for \0 */ + actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s", + prefix_len, prefix, namelen, name); + + if (actual_len < combined_len) + xfs_warn(context->dp->i_mount, + "cannot completely copy context buffer resulting in truncation"); compute_size: context->count += prefix_len + namelen + 1; --- > > How was this tested? With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/ but using scripts + image from: https://github.com/tytso/xfstests-bld here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the 5 default ones (I think?): | Ran: generic/475 generic/476 generic/521 generic/522 generic/642 | Passed all 5 tests Thanks Justin
On Tue, Apr 9, 2024 at 5:23 PM Justin Stitt <justinstitt@google.com> wrote: > > Hi, > > On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Fri, Apr 05, 2024 at 07:45:08PM +0000, Justin Stitt wrote: > > > - memcpy(offset, prefix, prefix_len); > > > - offset += prefix_len; > > > - strncpy(offset, (char *)name, namelen); /* real name */ > > > - offset += namelen; > > > - *offset = '\0'; > > > + > > > + combined_len = prefix_len + namelen; > > > + > > > + /* plus one byte for \0 */ > > > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); > > > + > > > + if (actual_len < combined_len) > > > > Shouldn't this be a != ? > > I guess it could be. It's a truncation check so I figured just > checking if the amount of bytes actually copied was less than the > total would suffice. > > > > > That being said I think this is actually wrong - the attr names are > > not NULL-terminated on disk, which is why we have the explicit > > zero terminataion above. > > Gotcha, in which case we could use the "%.*s" format specifier which > allows for a length argument. Does something like this look better? > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > index 364104e1b38a..1b7e886e0f29 100644 > --- a/fs/xfs/xfs_xattr.c > +++ b/fs/xfs/xfs_xattr.c > @@ -206,6 +206,7 @@ __xfs_xattr_put_listent( > { > char *offset; > int arraytop; > + size_t combined_len, actual_len; > > if (context->count < 0 || context->seen_enough) > return; > @@ -220,11 +221,16 @@ __xfs_xattr_put_listent( > return; > } > offset = context->buffer + context->count; > - memcpy(offset, prefix, prefix_len); > - offset += prefix_len; > - strncpy(offset, (char *)name, namelen); /* real name */ > - offset += namelen; > - *offset = '\0'; > + > + combined_len = prefix_len + namelen; > + > + /* plus one byte for \0 */ > + actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s", > + prefix_len, prefix, namelen, name); > + > + if (actual_len < combined_len) > + xfs_warn(context->dp->i_mount, > + "cannot completely copy context buffer resulting in truncation"); > > compute_size: > context->count += prefix_len + namelen + 1; > --- I copy pasted from vim -> gmail and it completely ate all my tabs. When I actually send the new patch, if needed, it will be formatted correctly :) > > > > > > > How was this tested? > > With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/ > > but using scripts + image from: https://github.com/tytso/xfstests-bld > > here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the > 5 default ones (I think?): > > | Ran: generic/475 generic/476 generic/521 generic/522 generic/642 > | Passed all 5 tests > > Thanks > Justin
On Tue, Apr 09, 2024 at 05:27:34PM -0700, Justin Stitt wrote: > On Tue, Apr 9, 2024 at 5:23 PM Justin Stitt <justinstitt@google.com> wrote: > > > > Hi, > > > > On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > On Fri, Apr 05, 2024 at 07:45:08PM +0000, Justin Stitt wrote: > > > > - memcpy(offset, prefix, prefix_len); > > > > - offset += prefix_len; > > > > - strncpy(offset, (char *)name, namelen); /* real name */ > > > > - offset += namelen; > > > > - *offset = '\0'; > > > > + > > > > + combined_len = prefix_len + namelen; > > > > + > > > > + /* plus one byte for \0 */ > > > > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); > > > > + > > > > + if (actual_len < combined_len) > > > > > > Shouldn't this be a != ? > > > > I guess it could be. It's a truncation check so I figured just > > checking if the amount of bytes actually copied was less than the > > total would suffice. > > > > > > > > That being said I think this is actually wrong - the attr names are > > > not NULL-terminated on disk, which is why we have the explicit > > > zero terminataion above. > > > > Gotcha, in which case we could use the "%.*s" format specifier which > > allows for a length argument. Does something like this look better? > > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > index 364104e1b38a..1b7e886e0f29 100644 > > --- a/fs/xfs/xfs_xattr.c > > +++ b/fs/xfs/xfs_xattr.c > > @@ -206,6 +206,7 @@ __xfs_xattr_put_listent( > > { > > char *offset; > > int arraytop; > > + size_t combined_len, actual_len; > > > > if (context->count < 0 || context->seen_enough) > > return; > > @@ -220,11 +221,16 @@ __xfs_xattr_put_listent( > > return; > > } > > offset = context->buffer + context->count; > > - memcpy(offset, prefix, prefix_len); > > - offset += prefix_len; > > - strncpy(offset, (char *)name, namelen); /* real name */ > > - offset += namelen; > > - *offset = '\0'; > > + > > + combined_len = prefix_len + namelen; > > + > > + /* plus one byte for \0 */ > > + actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s", > > + prefix_len, prefix, namelen, name); > > + > > + if (actual_len < combined_len) > > + xfs_warn(context->dp->i_mount, > > + "cannot completely copy context buffer resulting in truncation"); > > > > compute_size: > > context->count += prefix_len + namelen + 1; > > --- > > I copy pasted from vim -> gmail and it completely ate all my tabs. > When I actually send the new patch, if needed, it will be formatted > correctly :) Yeah, the "%.*s" version looks better. > > > > > > > > > > > > How was this tested? > > > > With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/ > > > > but using scripts + image from: https://github.com/tytso/xfstests-bld > > > > here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the > > 5 default ones (I think?): > > > > | Ran: generic/475 generic/476 generic/521 generic/522 generic/642 > > | Passed all 5 tests Would you mind adding "-g attr,label" into the mix so that you're running all the functional tests for xattr and fs label functionality? --D > > > > Thanks > > Justin >
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index 364104e1b38a..bc7246eaebdd 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -206,6 +206,7 @@ __xfs_xattr_put_listent( { char *offset; int arraytop; + size_t combined_len, actual_len; if (context->count < 0 || context->seen_enough) return; @@ -220,11 +221,16 @@ __xfs_xattr_put_listent( return; } offset = context->buffer + context->count; - memcpy(offset, prefix, prefix_len); - offset += prefix_len; - strncpy(offset, (char *)name, namelen); /* real name */ - offset += namelen; - *offset = '\0'; + + combined_len = prefix_len + namelen; + + /* plus one byte for \0 */ + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); + + if (actual_len < combined_len) + xfs_warn(context->dp->i_mount, + "cannot completely copy %s%s to context buffer resulting in truncation", + prefix, name); compute_size: context->count += prefix_len + namelen + 1;
strncpy is deprecated and as such we should prefer less ambiguous and more robust string interfaces [1]. There's a lot of manual memory management to get a prefix and name into a string. Let's use an easier to understand and more robust interface in scnprintf() to accomplish the same task while enabling us to check for possible truncation, resulting in a soft warning. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Split from https://lore.kernel.org/all/20240401-strncpy-fs-xfs-xfs_ioctl-c-v1-1-02b9feb1989b@google.com/ with feedback from Christoph H. --- fs/xfs/xfs_xattr.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) --- base-commit: c85af715cac0a951eea97393378e84bb49384734 change-id: 20240405-strncpy-xattr-split2-0a3aff0c6a20 Best regards, -- Justin Stitt <justinstitt@google.com>