[PATCH] autofs4: Do not potentially dereference NULL pointer returned by fget() in autofs_dev_ioctl_setpipefd()

[PATCH] autofs4: Do not potentially dereference NULL pointer returned by fget() in autofs_dev_ioctl_setpipefd()

[Jesper Juhl]

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.

When I originally submitted this patch I had chosen EBUSY as the return 
value to use if this happens. Ian Kent was kind enough to explain why that 
would most likely be wrong and why EBADF should most likely be used 
instead. This version of the patch uses EBADF.


Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 dev-ioctl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index eff9a41..a650d7e 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 = -EBADF;
+			goto out;
+		}
 		if (!pipe->f_op || !pipe->f_op->write) {
 			err = -EPIPE;
 			fput(pipe);



-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] autofs4: Do not potentially dereference NULL pointer returned by fget() in autofs_dev_ioctl_setpipefd()

[Ian Kent]

On Sat, 2010-12-18 at 22:43 +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.
> 
> When I originally submitted this patch I had chosen EBUSY as the return 
> value to use if this happens. Ian Kent was kind enough to explain why that 
> would most likely be wrong and why EBADF should most likely be used 
> instead. This version of the patch uses EBADF.
> 
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
Acked-by: Ian Kent <raven@themaw.net>

> ---
>  dev-ioctl.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> index eff9a41..a650d7e 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 = -EBADF;
> +			goto out;
> +		}
>  		if (!pipe->f_op || !pipe->f_op->write) {
>  			err = -EPIPE;
>  			fput(pipe);
> 
> 
> 

Re: [PATCH] autofs4: Do not potentially dereference NULL pointer returned by fget() in autofs_dev_ioctl_setpipefd()

[Jesper Juhl]

Hi Ian,

On Sat, 18 Dec 2010, 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.
> 
> When I originally submitted this patch I had chosen EBUSY as the return 
> value to use if this happens. Ian Kent was kind enough to explain why that 
> would most likely be wrong and why EBADF should most likely be used 
> instead. This version of the patch uses EBADF.
> 
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  dev-ioctl.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> index eff9a41..a650d7e 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 = -EBADF;
> +			goto out;
> +		}
>  		if (!pipe->f_op || !pipe->f_op->write) {
>  			err = -EPIPE;
>  			fput(pipe);
> 

It's been more than a month now since I submitted this updated patch 
adressing your feedback, but I've not seen any feedback on it.
Is it OK? Will you merge it? 

/Jesper


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] autofs4: Do not potentially dereference NULL pointer returned by fget() in autofs_dev_ioctl_setpipefd()

[Jesper Juhl]

On Mon, 24 Jan 2011, Jesper Juhl wrote:

> Hi Ian,
> 
> On Sat, 18 Dec 2010, 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.
> > 
> > When I originally submitted this patch I had chosen EBUSY as the return 
> > value to use if this happens. Ian Kent was kind enough to explain why that 
> > would most likely be wrong and why EBADF should most likely be used 
> > instead. This version of the patch uses EBADF.
> > 
> > 
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > ---
> >  dev-ioctl.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> > index eff9a41..a650d7e 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 = -EBADF;
> > +			goto out;
> > +		}
> >  		if (!pipe->f_op || !pipe->f_op->write) {
> >  			err = -EPIPE;
> >  			fput(pipe);
> > 
> 
> It's been more than a month now since I submitted this updated patch 
> adressing your feedback, but I've not seen any feedback on it.
> Is it OK? Will you merge it? 
> 
Ok, I need to learn to search my mailbox better. I just saw that you did 
indeed send a reply with an Acked-by: on december 28. 

Doesn't change the fact that I still need to find someone to actually 
merge it...

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] autofs4: Do not potentially dereference NULL pointer returned by fget() in autofs_dev_ioctl_setpipefd()

[Ian Kent]

On Mon, 2011-01-24 at 21:03 +0100, Jesper Juhl wrote:
> On Mon, 24 Jan 2011, Jesper Juhl wrote:
> 
> > Hi Ian,
> > 
> > On Sat, 18 Dec 2010, 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.
> > > 
> > > When I originally submitted this patch I had chosen EBUSY as the return 
> > > value to use if this happens. Ian Kent was kind enough to explain why that 
> > > would most likely be wrong and why EBADF should most likely be used 
> > > instead. This version of the patch uses EBADF.
> > > 
> > > 
> > > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > > ---
> > >  dev-ioctl.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> > > index eff9a41..a650d7e 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 = -EBADF;
> > > +			goto out;
> > > +		}
> > >  		if (!pipe->f_op || !pipe->f_op->write) {
> > >  			err = -EPIPE;
> > >  			fput(pipe);
> > > 
> > 
> > It's been more than a month now since I submitted this updated patch 
> > adressing your feedback, but I've not seen any feedback on it.
> > Is it OK? Will you merge it? 
> > 
> Ok, I need to learn to search my mailbox better. I just saw that you did 
> indeed send a reply with an Acked-by: on december 28. 
> 
> Doesn't change the fact that I still need to find someone to actually 
> merge it...
> 

Often patches like this get merged whether I ack them or not.

Right now I'm struggling with the 2.6.38-rc changes since the merge of
vfs-scale together with the vfs-automount patches caused some breakage.

I will post it along with any other patches I end up with if it doesn't
get picked up beforehand.

Ian