lkml.org 
[lkml]   [2006]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH][RFC][resend] potential NULL pointer deref in XFS on failed mount
    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....
    >

    Thanks a lot for commenting Dave.

    I don't have time to look more at this tonight or tomorrow, but I'll
    try to cook up a proper fix covering all cases during the weekend.

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

    \
     
     \ /
      Last update: 2006-11-16 23:21    [W:0.031 / U:30.392 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site