Re: [PATCH][RFC][resend] potential NULL pointer deref in XFS on failed mount

[Jesper Juhl]

On 16/11/06, Jesper Juhl <jesper.juhl@xxxxxxxxx> wrote:
> On 16/11/06, David Chinner <dgc@xxxxxxx> wrote:
> > On Thu, Nov 16, 2006 at 10:18:26PM +0100, Jesper Juhl wrote:
> > > (got no reply on this when I originally send it on 20061031, so resending
> > >  now that a bit of time has passed.  The patch still applies cleanly to
> > >  Linus' git tree as of today.)
> > >
> > >
> > > The Coverity checker spotted a potential problem in XFS.
> > >
> > > The problem is that if, in xfs_mount(), this code triggers:
> > >
> > >       ...
> > >       if (!mp->m_logdev_targp)
> > >               goto error0;
> > >       ...
> > >
> > > Then we'll end up calling xfs_unmountfs_close() with a NULL
> > > 'mp->m_logdev_targp'.
> > > This in turn will result in a call to xfs_free_buftarg() with its 'btp'
> > > argument == NULL. xfs_free_buftarg() dereferences 'btp' leading to
> > > a NULL pointer dereference and crash.
> >
> > Interesting that coverity found that, but failed to find the other
> > leaks in that function from exactly the same code and error
> > case.....
> >
> > > I think this can happen, since the fatal call to xfs_free_buftarg()
> > > happens when 'm_logdev_targp != m_ddev_targp' and due to a check of
> > > 'm_ddev_targp' against NULL in xfs_mount() (and subsequent return if it is
> > > NULL) the two will never both be NULL when we hit the error0 label from
> > > the two lines cited above.
> > >
> > > Comments welcome (please keep me on Cc: on replies).
> > >
> > > Here's a proposed patch to fix this by testing 'btp' against NULL in
> > > xfs_free_buftarg().
> >
> > Not the right fix - we should only be trying to free valid
> > buftargs, which means xfs_unmountfs_close() is the correct
> > place to fix this....
> >
>
> Ok.
>
> > e.g:
> >
> > -       if (mp->m_logdev_targp != mp->m_ddev_targp)
> > +       if (mp->m_logdev_targp && (mp->m_logdev_targp != mp->m_ddev_targp))
> >
> > As to the afore-mentioned leaks, if we fail to allocate a realtime
> > buftarg, then we will leak a reference to both the rtdev and logdev,
> > and if we fail to allocate an external log buftarg we'll leak a
> > reference to the logdev. i.e., we fail to do one or both of:
> >
> >         xfs_blkdev_put(logdev);
> >         xfs_blkdev_put(rtdev);
> >
> > To remove the bdev references we may have gained earlier. Normally,
> > these references are released by xfs_free_buftarg(), but because we
> > failed to allocate the buftarg, we can't drop the references via
> > that method....
> >

How about something like the attached patch ?

(sorry about the attachment, but I can't inline the patch from my
current location - a whitespace damaged version is below though for
easy review)



Signed-off-by: Jesper Juhl <jesper.juhl@xxxxxxxxx>
---

fs/xfs/xfs_mount.c  |    2 +-
fs/xfs/xfs_vfsops.c |   10 ++++++++--
2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9dfae18..3497128 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1135,7 +1135,7 @@ #endif
void
xfs_unmountfs_close(xfs_mount_t *mp, struct cred *cr)
{
-       if (mp->m_logdev_targp != mp->m_ddev_targp)
+       if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
               xfs_free_buftarg(mp->m_logdev_targp, 1);
       if (mp->m_rtdev_targp)
               xfs_free_buftarg(mp->m_rtdev_targp, 1);
diff --git a/fs/xfs/xfs_vfsops.c b/fs/xfs/xfs_vfsops.c
index 62336a4..6d7e8f1 100644
--- a/fs/xfs/xfs_vfsops.c
+++ b/fs/xfs/xfs_vfsops.c
@@ -473,13 +473,19 @@ xfs_mount(
       }
       if (rtdev) {
               mp->m_rtdev_targp = xfs_alloc_buftarg(rtdev, 1);
-               if (!mp->m_rtdev_targp)
+               if (!mp->m_rtdev_targp) {
+                       xfs_blkdev_put(logdev);
+                       xfs_blkdev_put(rtdev);
                       goto error0;
+               }
       }
       mp->m_logdev_targp = (logdev && logdev != ddev) ?
                               xfs_alloc_buftarg(logdev, 1) : mp->m_ddev_targp;
-       if (!mp->m_logdev_targp)
+       if (!mp->m_logdev_targp) {
+               xfs_blkdev_put(logdev);
+               xfs_blkdev_put(rtdev);
               goto error0;
+       }

       /*
        * Setup flags based on mount(2) options and then the superblock


--
Jesper Juhl <jesper.juhl@xxxxxxxxx>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Signed-off-by: Jesper Juhl <jesper.juhl@xxxxxxxxx>
---

 fs/xfs/xfs_mount.c  |    2 +-
 fs/xfs/xfs_vfsops.c |   10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9dfae18..3497128 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1135,7 +1135,7 @@ #endif
 void
 xfs_unmountfs_close(xfs_mount_t *mp, struct cred *cr)
 {
-	if (mp->m_logdev_targp != mp->m_ddev_targp)
+	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
 		xfs_free_buftarg(mp->m_logdev_targp, 1);
 	if (mp->m_rtdev_targp)
 		xfs_free_buftarg(mp->m_rtdev_targp, 1);
diff --git a/fs/xfs/xfs_vfsops.c b/fs/xfs/xfs_vfsops.c
index 62336a4..6d7e8f1 100644
--- a/fs/xfs/xfs_vfsops.c
+++ b/fs/xfs/xfs_vfsops.c
@@ -473,13 +473,19 @@ xfs_mount(
 	}
 	if (rtdev) {
 		mp->m_rtdev_targp = xfs_alloc_buftarg(rtdev, 1);
-		if (!mp->m_rtdev_targp)
+		if (!mp->m_rtdev_targp) {
+			xfs_blkdev_put(logdev);
+			xfs_blkdev_put(rtdev);
 			goto error0;
+		}
 	}
 	mp->m_logdev_targp = (logdev && logdev != ddev) ?
 				xfs_alloc_buftarg(logdev, 1) : mp->m_ddev_targp;
-	if (!mp->m_logdev_targp)
+	if (!mp->m_logdev_targp) {
+		xfs_blkdev_put(logdev);
+		xfs_blkdev_put(rtdev);
 		goto error0;
+	}
 
 	/*
 	 * Setup flags based on mount(2) options and then the superblock