linux-kernel - Re: [PATCH upstream] KASAN: slab-out-of-bounds Read in getname

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1530495726.2749.13.camel@themaw.net>
Date:   Mon, 02 Jul 2018 09:42:06 +0800
From:   Ian Kent <raven@...maw.net>
To:     tomas <tomasbortoli@...il.com>
Cc:     linux-kernel@...r.kernel.org, syzkaller@...glegroups.com,
        autofs@...r.kernel.org
Subject: Re: [PATCH upstream] KASAN: slab-out-of-bounds Read in
 getname_kernel

On Mon, 2018-07-02 at 09:10 +0800, Ian Kent wrote:
> On Mon, 2018-07-02 at 00:04 +0200, tomas wrote:
> > Hi,
> > 
> > I've looked into this issue found by Syzbot and I made a patch:
> > 
> > https://syzkaller.appspot.com/bug?id=d03abd8b42847f7f69b1d1d7f97208ae425b116
> > 3
> 
> Umm ... oops!
> 
> Thanks for looking into this Tomas.
> 
> > 
> > 
> > The autofs subsystem does not check that the "path" parameter is present
> > within the "param" struct passed by the userspace in case the
> > AUTOFS_DEV_IOCTL_OPENMOUNT_CMD command is passed. Indeed, it assumes a
> > path is always provided (though a path is not always present, as per how
> > the struct is defined:
> > https://github.com/torvalds/linux/blob/master/include/uapi/linux/auto_dev-io
> > ct
> > l.h#L89).
> > Skipping the check provokes an oob read in "strlen", called by
> > "getname_kernel", in turn called by the autofs to assess the length of
> > the non-existing path.
> > 
> > To solve it, modify the "validate_dev_ioctl" function to check also that
> > a path has been provided if the command is AUTOFS_DEV_IOCTL_OPENMOUNT_CMD.
> > 
> > 
> > --- b/fs/autofs/dev-ioctl.c    2018-07-01 23:10:16.059728621 +0200
> > +++ a/fs/autofs/dev-ioctl.c    2018-07-01 23:10:24.311792133 +0200
> > @@ -136,6 +136,9 @@ static int validate_dev_ioctl(int cmd, s
> >              goto out;
> >          }
> >      }
> > +    /* AUTOFS_DEV_IOCTL_OPENMOUNT_CMD without path */
> > +    else if(_IOC_NR(cmd) == AUTOFS_DEV_IOCTL_OPENMOUNT_CMD)
> > +        return -EINVAL;
> 
> My preference is to put the comment inside the else but ...
> 
> There's another question, should the check be done in
> autofs_dev_ioctl_openmount() in the same way it's checked in other
> ioctls that need a path, such as in autofs_dev_ioctl_requester()
> and autofs_dev_ioctl_ismountpoint()?
> 
> For consistency I'd say it should.
> 
> >  
> >      err = 0;
> >  out:
> > 
> > 
> > Tested and solves the issue on Linus' main git tree.
> > 
> > 

Or perhaps this (not even compile tested) patch would be better?

autofs - fix slab out of bounds read in getname_kernel()

From: Ian Kent <raven@...maw.net>

The autofs subsystem does not check that the "path" parameter is
present for all cases where it is required when it is passed in
via the "param" struct.

In particular it isn't checked for the AUTOFS_DEV_IOCTL_OPENMOUNT_CMD
ioctl command.

To solve it, modify validate_dev_ioctl() function to check that a
path has been provided for ioctl commands that require it.
---
 fs/autofs/dev-ioctl.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/autofs/dev-ioctl.c b/fs/autofs/dev-ioctl.c
index ea4ca1445ab7..61c63715c3fb 100644
--- a/fs/autofs/dev-ioctl.c
+++ b/fs/autofs/dev-ioctl.c
@@ -135,6 +135,11 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
 				cmd);
 			goto out;
 		}
+	} else if (cmd == AUTOFS_DEV_IOCTL_OPENMOUNT_CMD ||
+		   cmd == AUTOFS_DEV_IOCTL_REQUESTER_CMD ||
+		   cmd == AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD) {
+		err = -EINVAL;
+		goto out;
 	}
 
 	err = 0;
@@ -433,10 +438,7 @@ static int autofs_dev_ioctl_requester(struct file *fp,
 	dev_t devid;
 	int err = -ENOENT;
 
-	if (param->size <= AUTOFS_DEV_IOCTL_SIZE) {
-		err = -EINVAL;
-		goto out;
-	}
+	/* param->path has already been checked */
 
 	devid = sbi->sb->s_dev;
 
@@ -521,10 +523,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 	unsigned int devid, magic;
 	int err = -ENOENT;
 
-	if (param->size <= AUTOFS_DEV_IOCTL_SIZE) {
-		err = -EINVAL;
-		goto out;
-	}
+	/* param->path has already been checked */
 
 	name = param->path;
 	type = param->ismountpoint.in.type;