On Tue, Sep 15, 2020 at 10:05:53AM -0400, Jeff Layton wrote:
> > > +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> > > + struct delayed_call *done)
> > > +{
> > > + struct ceph_inode_info *ci = ceph_inode(inode);
> > > +
> > > + if (!dentry)
> > > + return ERR_PTR(-ECHILD);
> > > +
> > > + return fscrypt_get_symlink(inode, ci->i_symlink, ksize(ci->i_symlink), done);
> >
> > Using ksize() seems wrong here, since that would allow fscrypt_get_symlink() to
> > read beyond the part of the buffer that is actually initialized.
> >
>
> Is that actually a problem? I did have an earlier patch that carried
> around the length, but it didn't seem to be necessary.
>
> ISTM that that might end up decrypting more data than is actually
> needed, but eventually there will be a NULL terminator in the data and
> the rest would be ignored.
>
Yes it's a problem. The code that decrypts the symlink adds the null terminator
at the end. So if the stated buffer size is wrong, then decrypted uninitialized
memory can be included into the symlink target that userspace then sees.
> If it is a problem, then we should probably change the comment header
> over fscrypt_get_symlink. It currently says:
>
> * @max_size: size of @caddr buffer
>
> ...which is another reason why I figured using ksize there was OK.
ksize() is rarely used, as it should be. (For one, it disables KASAN on the
buffer...) I think that when people see "buffer size" they almost always think
the actual allocated size of the buffer, not ksize(). But we could change it to
say "allocated size" if that would make it clearer...
- Eric
|