linux-kernel - Re: [PATCH 01/19] vfs: syscall: Add fsinfo() to query filesystem information [ver #16]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Feb 2020 11:03:56 +0000
From:   David Howells <dhowells@...hat.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     dhowells@...hat.com, Al Viro <viro@...iv.linux.org.uk>,
        raven@...maw.net, Miklos Szeredi <mszeredi@...hat.com>,
        Christian Brauner <christian@...uner.io>,
        Linux API <linux-api@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/19] vfs: syscall: Add fsinfo() to query filesystem information [ver #16]

Jann Horn <jannh@...gle.com> wrote:

> > +int fsinfo_string(const char *s, struct fsinfo_context *ctx)
> ...
> Please add a check here to ensure that "ret" actually fits into the
> buffer (and use WARN_ON() if you think the check should never fire).
> Otherwise I think this is too fragile.

How about:

	int fsinfo_string(const char *s, struct fsinfo_context *ctx)
	{
		unsigned int len;
		char *p = ctx->buffer;
		int ret = 0;
		if (s) {
			len = strlen(s);
			if (len > ctx->buf_size - 1)
				len = ctx->buf_size;
			if (!ctx->want_size_only) {
				memcpy(p, s, len);
				p[len] = 0;
			}
			ret = len;
		}
		return ret;
	}

I've also added a check to eliminate the copy if userspace didn't actually
supply a buffer.

> > +       ret = vfs_statfs(path, &buf);
> > +       if (ret < 0 && ret != -ENOSYS)
> > +               return ret;
> ...
> > +       memcpy(&p->f_fsid, &buf.f_fsid, sizeof(p->f_fsid));
> 
> What's going on here? If vfs_statfs() returns -ENOSYS, we just use the
> (AFAICS uninitialized) buf.f_fsid anyway in the memcpy() below and
> return it to userspace?

Good point.  I've made the access to the buffer contingent on ret==0.  If I
don't set it, it will just be left pre-cleared.

> > +       return sizeof(*attr);
> 
> I think you meant sizeof(*info).

Yes.  I've renamed the buffer point to "p" in all cases so that it's more
obvious.

> > +       return ctx->usage;
> 
> It is kind of weird that you have to return the ctx->usage everywhere
> even though the caller already has ctx...

At this point, it's only used and returned by fsinfo_attributes() and really
is only for the use of the attribute getter function.

I could, I suppose, return the amount of data in ctx->usage and then preset it
for VSTRUCT-type objects.  Unfortunately, I can't make the getter return void
since it might have to return an error.

> > +               ctx->buffer = kvmalloc(ctx->buf_size, GFP_KERNEL);
> 
> ctx->buffer is _almost_ always pre-zeroed (see vfs_do_fsinfo() below),
> except if we have FSINFO_TYPE_OPAQUE or FSINFO_TYPE_LIST with a size
> bigger than what the attribute's ->size field said? Is that
> intentional?

Fixed.

> > +struct fsinfo_attribute {
> > +       unsigned int            attr_id;        /* The ID of the attribute */
> > +       enum fsinfo_value_type  type:8;         /* The type of the attribute's value(s) */
> > +       unsigned int            flags:8;
> > +       unsigned int            size:16;        /* - Value size (FSINFO_STRUCT) */
> > +       unsigned int            element_size:16; /* - Element size (FSINFO_LIST) */
> > +       int (*get)(struct path *path, struct fsinfo_context *params);
> > +};
> 
> Why the bitfields? It doesn't look like that's going to help you much,
> you'll just end up with 6 bytes of holes on x86-64:

Expanding them to non-bitfields will require an extra 10 bytes, making the
struct 8 bytes bigger with 4 bytes of padding.  I can do that if you'd rather.

David