Re: [PATCH v2] NFS: Avoid cross-structure casting

[Trond Myklebust]

On Wed, 2017-04-12 at 11:59 -0700, Kees Cook wrote:
> On Wed, Apr 5, 2017 at 8:29 AM, Kees Cook <keescook@xxxxxxxxxxxx>
> wrote:
> > When the call to nfs_devname() fails, the error path attempts to
> > retain
> > the error via the mnt variable, but this requires a cast across
> > very
> > different types (char * to struct vfsmount *), which the upcoming
> > structure layout randomization plugin flags as being potentially
> > dangerous in the face of randomization. This is a false positive,
> > but
> > what this code actually wants to do is retain the error value, so
> > this
> > patch explicitly sets it, instead of using what seems to be an
> > unexpected cast.
> > 
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> 
> If I can get an Acked-by on this, I could push it via the gcc-plugin
> tree.
> 
> Thanks!
> 
> -Kees
> 
> > ---
> > v2: duh, use ERR_CAST. thanks neilb!
> > ---
> > Âfs/nfs/namespace.c | 5 +++--
> > Â1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > index 786f17580582..8ca5d147124d 100644
> > --- a/fs/nfs/namespace.c
> > +++ b/fs/nfs/namespace.c
> > @@ -259,9 +259,10 @@ struct vfsmount *nfs_do_submount(struct dentry
> > *dentry, struct nfs_fh *fh,
> > ÂÂÂÂÂÂÂÂif (page == NULL)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto out;
> > ÂÂÂÂÂÂÂÂdevname = nfs_devname(dentry, page, PAGE_SIZE);
> > -ÂÂÂÂÂÂÂmnt = (struct vfsmount *)devname;
> > -ÂÂÂÂÂÂÂif (IS_ERR(devname))
> > +ÂÂÂÂÂÂÂif (IS_ERR(devname)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmnt = ERR_CAST(devname);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto free_page;
> > +ÂÂÂÂÂÂÂ}
> > ÂÂÂÂÂÂÂÂmnt = nfs_do_clone_mount(NFS_SB(dentry->d_sb), devname,
> > &mountdata);
> > Âfree_page:
> > ÂÂÂÂÂÂÂÂfree_page((unsigned long)page);
> > --
> > 2.7.4
> > 
> > 
> > --
> > Kees Cook
> > Pixel Security
> 

Acked-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@xxxxxxxxxxxxxxx