Re: [PATCH] cachefiles: Fix KASAN slab-out-of-bounds in cachefiles_set_volume_xattr

[David Wysochanski <dwysocha@redhat.com>]

On Tue, Apr 5, 2022 at 10:24 AM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Tue, Apr 5, 2022 at 10:14 AM David Howells <dhowells@redhat.com> wrote:
> >
> > Dave Wysochanski <dwysocha@redhat.com> wrote:
> >
> > > @@ -203,7 +203,7 @@ bool cachefiles_set_volume_xattr(struct cachefiles_volume *volume)
> > >       if (!buf)
> > >               return false;
> > >       buf->reserved = cpu_to_be32(0);
> > > -     memcpy(buf->data, p, len);
> > > +     memcpy(buf->data, p, volume->vcookie->coherency_len);
> >
> > Good catch.  However, I think it's probably better to change things a bit
> > further up, eg.:
> >
> >         -       len += sizeof(*buf);
> >         -       buf = kmalloc(len, GFP_KERNEL);
> >         +       buf = kmalloc(sizeof(*buf) + len, GFP_KERNEL);
> >
> > David
> >
>
> Agree with the above.  I'll send a v2.  Thanks!

After I looked at this again I realized 'len' is used in
vfs_setxattr() and needs to be the size of the kmalloc'd memory.
So we need another adjustment there or the v1 patch.

    191 bool cachefiles_set_volume_xattr(struct cachefiles_volume *volume)
    192 {
    193         struct cachefiles_vol_xattr *buf;
    194         unsigned int len = volume->vcookie->coherency_len;
    195         const void *p = volume->vcookie->coherency;
    196         struct dentry *dentry = volume->dentry;
    197         int ret;
    198
    199         _enter("%x,#%d", volume->vcookie->debug_id, len);
    200
    201         buf = kmalloc(sizeof(*buf) + len, GFP_KERNEL);
    202         if (!buf)
    203                 return false;
    204         buf->reserved = cpu_to_be32(0);
    205         memcpy(buf->data, p, len);
    206
    207         ret = cachefiles_inject_write_error();
    208         if (ret == 0)
    209                 ret = vfs_setxattr(&init_user_ns, dentry,
cachefiles_xattr_cache,
    210                                    buf, len, 0);