Jann Horn <jannh@xxxxxxxxxx> 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
