On Tue, Apr 03, 2018 at 12:30:57PM -0500, Eric Sandeen wrote:
> On 3/20/18 10:39 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Avoid a buffer overflow when we're formatting extended attribute names
> > for name checking.
>
> This won't /actually/ overflow, right, because you are doing
> snprintf(NAME_MAX) into a buffer of size [NAME_MAX + 1].
>
> However, it might truncate the attribute string if too long.
> (just trying to avoid security folk wig-outs).
>
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > scrub/phase5.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> >
> > diff --git a/scrub/phase5.c b/scrub/phase5.c
> > index 8e0a1be..36821d0 100644
> > --- a/scrub/phase5.c
> > +++ b/scrub/phase5.c
> > @@ -143,6 +143,8 @@ static const struct xfs_attr_ns attr_ns[] = {
> > {ATTR_SECURE, "secure"},
> > {0, NULL},
> > };
> > +/* Enough space to handle the prefix. */
> > +#define ATTR_NAME_MAX (NAME_MAX + 8)
>
> Unrelated to this change, really, but should NAME_MAX be
> XATTR_NAME_MAX for clarity & consistency w/ the xattr code?
>
> (it's defined in /usr/include/linux/limits.h)
Hm, I got bogged down here trying to figure out if XATTR_NAME_MAX
applied to the entire string "security.foo" or just the ".foo" part.
The answer is (according to getxattr()) the first, so I'll fix this to
use XATTR_NAME_MAX directly...
>
> >
> > /*
> > * Check all the xattr names in a particular namespace of a file handle
> > @@ -158,7 +160,7 @@ xfs_scrub_scan_fhandle_namespace_xattrs(
> > {
> > struct attrlist_cursor cur;
> > char attrbuf[XFS_XATTR_LIST_MAX];
> > - char keybuf[NAME_MAX + 1];
> > + char keybuf[ATTR_NAME_MAX + 1];
> > struct attrlist *attrlist = (struct attrlist *)attrbuf;
> > struct attrlist_ent *ent;
> > struct unicrash *uc;
> > @@ -172,14 +174,14 @@ xfs_scrub_scan_fhandle_namespace_xattrs(
> >
> > memset(attrbuf, 0, XFS_XATTR_LIST_MAX);
> > memset(&cur, 0, sizeof(cur));
> > - memset(keybuf, 0, NAME_MAX + 1);
> > + memset(keybuf, 0, ATTR_NAME_MAX + 1);
> > error = attr_list_by_handle(handle, sizeof(*handle), attrbuf,
> > XFS_XATTR_LIST_MAX, attr_ns->flags, &cur);
> > while (!error) {
> > /* Examine the xattrs. */
> > for (i = 0; i < attrlist->al_count; i++) {
> > ent = ATTR_ENTRY(attrlist, i);
> > - snprintf(keybuf, NAME_MAX, "%s.%s", attr_ns->name,
>
> To future proof this rather than relying on a hardcoded 8 based on the
> current namespaces above, how about:
>
> #define XATTR_NS_MAX 8
> #define ATTR_STRING_MAX (XATTR_NS_MAX + XATTR_NAME_MAX + 1) /* for '.' */
>
> char keybuf[ATTR_STRING_MAX + 1];
...and then we can use it directly here instead of all these weird
NAME_MAX variants.
>
> ASSERT(strlen(attr_ns->name) <= XATTR_NS_MAX);
> snprintf(keybuf, ATTR_STRING_MAX, "%s.%s", attr_ns->name,
> ent->a_name);
>
> just in case someone adds a new
>
> ATTR_SUPERDELUXE, "superdeluxe"
>
> namespace some day? Is that overkill?
No, not overkill.
--D
>
> > + snprintf(keybuf, ATTR_NAME_MAX, "%s.%s", attr_ns->name,
> > ent->a_name);
> > moveon = xfs_scrub_check_name(ctx, descr,
> > _("extended attribute"), keybuf);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
