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  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]
Date:	Thu, 14 May 2015 13:43:04 +0200
From:	Jan Kara <jack@...e.cz>
To:	Lukáš Czerner <lczerner@...hat.com>
Cc:	Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org,
	Theodore Ts'o <tytso@....edu>, stable@...r.kernel.org
Subject: Re: [PATCH v2] ext4: fix NULL pointer dereference when journal
 restart fails

On Thu 14-05-15 13:33:28, Lukáš Czerner wrote:
> On Thu, 14 May 2015, Jan Kara wrote:
> 
> > Date: Thu, 14 May 2015 12:35:00 +0200
> > From: Jan Kara <jack@...e.cz>
> > To: Lukáš Czerner <lczerner@...hat.com>
> > Cc: Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org,
> >     Theodore Ts'o <tytso@....edu>, stable@...r.kernel.org
> > Subject: Re: [PATCH v2] ext4: fix NULL pointer dereference when journal
> >     restart fails
> > 
> > On Thu 14-05-15 11:37:46, Lukáš Czerner wrote:
> > > On Thu, 14 May 2015, Jan Kara wrote:
> > > 
> > > > Date: Thu, 14 May 2015 11:24:57 +0200
> > > > From: Jan Kara <jack@...e.cz>
> > > > To: Lukas Czerner <lczerner@...hat.com>
> > > > Cc: linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
> > > >     Jan Kara <jack@...e.cz>, stable@...r.kernel.org
> > > > Subject: Re: [PATCH v2] ext4: fix NULL pointer dereference when journal
> > > >     restart fails
> > > > 
> > > > On Thu 14-05-15 11:03:06, Lukas Czerner wrote:
> > > > > Currently when journal restart fails, we'll have the h_transaction of
> > > > > the handle set to NULL to indicate that the handle has been effectively
> > > > > aborted. We handle this situation quietly in the jbd2_journal_stop() and just
> > > > > free the handle and exit because everything else has been done before we
> > > > > attempted (and failed) to restart the journal.
> > > > > 
> > > > > Unfortunately there are a number of problems with that approach
> > > > > introduced with commit
> > > > > 
> > > > > 41a5b913197c "jbd2: invalidate handle if jbd2_journal_restart()
> > > > > fails"
> > > > > 
> > > > > First of all in ext4 jbd2_journal_stop() will be called through
> > > > > __ext4_journal_stop() where we would try to get a hold of the superblock
> > > > > by dereferencing h_transaction which in this case would lead to NULL
> > > > > pointer dereference and crash.
> > > > > 
> > > > > In addition we're going to free the handle regardless of the refcount
> > > > > which is bad as well, because others up the call chain will still
> > > > > reference the handle so we might potentially reference already freed
> > > > > memory.
> > > > > 
> > > > > Moreover it's expected that we'll get aborted handle as well as detached
> > > > > handle in some of the journalling function as the error propagates up
> > > > > the stack, so it's unnecessary to call WARN_ON every time we get
> > > > > detached handle.
> > > > > 
> > > > > And finally we might leak some memory by forgetting to free reserved
> > > > > handle in jbd2_journal_stop() in the case where handle was detached from
> > > > > the transaction (h_transaction is NULL).
> > > > > 
> > > > > Fix the NULL pointer dereference in __ext4_journal_stop() by just
> > > > > calling jbd2_journal_stop() quietly as suggested by Jan Kara. Also fix
> > > > > the potential memory leak in jbd2_journal_stop() and use proper
> > > > > handle refcounting before we attempt to free it to avoid use-after-free
> > > > > issues.
> > > > > 
> > > > > And finally remove all WARN_ON(!transaction) from the code so that we do
> > > > > not get random traces when something goes wrong because when journal
> > > > > restart fails we will get to some of those functions.
> > > > > 
> > > > > Signed-off-by: Lukas Czerner <lczerner@...hat.com>
> > > > > Cc: stable@...r.kernel.org
> > > > > ---
> > > > > v2: As Jan Kara pointed out setting h_transaction to NULL is actually
> > > > > desirable because the handle was detached from that transaction.
> > > > 
> > > > The patch looks good, you can add:
> > > > 
> > > > Reviewed-by: Jan Kara <jack@...e.cz>
> > > > 
> > > >   Just one small nit below:
> > > > 
> > > > ...
> > > > > @@ -1530,8 +1524,22 @@ int jbd2_journal_stop(handle_t *handle)
> > > > >  	tid_t tid;
> > > > >  	pid_t pid;
> > > > >  
> > > > > -	if (!transaction)
> > > > > -		goto free_and_exit;
> > > > > +	if (!transaction) {
> > > > > +		/*
> > > > > +		 * Handle is already detached from the transaction so
> > > > > +		 * there is nothing to do other than decrease a refcount,
> > > > > +		 * or free the handle if refcount drops to zero
> > > > > +		 */
> > > > > +		if (--handle->h_ref > 0) {
> > > > > +			jbd_debug(4, "h_ref %d -> %d\n", handle->h_ref + 1,
> > > > > +							 handle->h_ref);
> > > > > +			return err;
> > > > > +		} else {
> > > > > +			if (handle->h_rsv_handle)
> > > > > +				jbd2_free_handle(handle->h_rsv_handle);
> > > > > +			goto free_and_exit;
> > > > 
> > > > It would we nicer if you just moved free_and_exit label before freeing of
> > > > the reserved handle instead of duplicating the code here. Also it is more
> > > > future proof if we add more places jumping to the label...
> > > 
> > > The problem is that this a special case when we have detached handle
> > > so we only need to free the structure. In normal case we're calling
> > > jbd2_journal_free_reserved() instead and I did not wanted to add
> > > (!transaction) condition to multiple places.
> > > 
> > > jbd2_journal_free_reserved() is different in that it does
> > > sub_reserved_credits() for the journal, but in this case it has
> > > already been done in jbd2__journal_restart().
> >   Hum, actually you must call jbd2_journal_free_reserved() so that you
> > don't leak credits assigned for the reserved handle, I missed that you call
> > only jbd2_free_handle(). And note that although the current handle is
> > detached, its reserved handle is still properly attached to the journal
> > (not any particular transaction) so it is safe to call
> > jbd2_journal_free_reserved().
> 
> Sure, but this is already done in jbd2__journal_restart()
> 
> if (handle->h_rsv_handle) {
> 	sub_reserved_credits(journal, handle->h_rsv_handle->h_buffer_credits);
> }
> 
> and AFAICT if start_this_handle() fails then we would not have
> increased the reserved credits, or am I missing something ?
  Argh, the code is more subtle than I thought ;-). You are absolutely
right and what you do is correct.

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists