Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | From | Justin Stitt <> | Date | Tue, 9 Apr 2024 17:23:35 -0700 | Subject | Re: [PATCH] xfs: xattr: replace strncpy and check for truncation |
| |
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
| ![\](/images/icornerr.gif) |