linux-kernel - Re: [autofs] [PATCH][RFC] autofs4: Do not potentially dereference null pointer returned by fget() in autofs_dev_ioctl

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: <1292382195.2979.17.camel@perseus>
Date:	Wed, 15 Dec 2010 11:03:15 +0800
From:	Ian Kent <raven@...maw.net>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	autofs@...ux.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [autofs] [PATCH][RFC] autofs4: Do not potentially dereference
 null pointer returned by fget() in autofs_dev_ioctl_setpipefd()

On Mon, 2010-12-13 at 00:02 +0100, Jesper Juhl wrote:
> Hi,
> 
> In fs/autofs4/dev-ioctl.c::autofs_dev_ioctl_setpipefd() we call fget(), 
> which may return NULL, but we do not explicitly test for that NULL return 
> so we may end up dereferencing a NULL pointer - bad.

Oops, thanks for this.

> 
> A comment in fget() says "File object ref couldn't be taken" when that 
> function returns NULL, so I guess EBUSY is the proper error to return from 
> autofs_dev_ioctl_setpipefd() when this happens, but I'm far from sure 
> about this, so I'd like some feedback before this patch is merged.

Not sure EBUSY is the one to use.

What is happening here is that user space has opened a file handle it
thinks is against an autofs mount point (which may actually be covered
by another mount, hence the need for an ioctl) and is asking the kernel
to set it in the super block info struct so it can be used for control
operations. By the look of fget() a NULL return would happen if the file
table was full (ENFILE) or the file handle had already been closed
(probably EBADF) so perhaps EBADF would be a better choice.

> 
> 
> Signed-off-by: Jesper Juhl <jj@...osbits.net>
> ---
>  dev-ioctl.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
>  compile tested only.
> 
> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> index eff9a41..ab551ee 100644
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -372,6 +372,10 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
>  		return -EBUSY;
>  	} else {
>  		struct file *pipe = fget(pipefd);
> +		if (!pipe) {
> +			err = -EBUSY;
> +			goto out;
> +		}
>  		if (!pipe->f_op || !pipe->f_op->write) {
>  			err = -EPIPE;
>  			fput(pipe);
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/