Jann Horn [off-list ref] wrote:

quoted

+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.

quoted

+       ret = vfs_statfs(path, &buf);
+       if (ret < 0 && ret != -ENOSYS)
+               return ret;

...

quoted

+       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.

quoted

+       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.

quoted

+       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.

quoted

+               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.

quoted

+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