Messages in this thread Patches in this message | | | Date | Mon, 20 Nov 2006 15:32:43 +0100 | From | "Jesper Juhl" <> | Subject | Re: [PATCH][RFC][resend] potential NULL pointer deref in XFS on failed mount |
| |
On 16/11/06, Jesper Juhl <jesper.juhl@gmail.com> wrote: > On 16/11/06, David Chinner <dgc@sgi.com> 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@gmail.com> ---
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@gmail.com> 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@gmail.com> ---
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 | |