linux-kernel - Re: [PATCH] ocfs2: Fix handle refcount leak in two exception handling paths

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <636c032f-b0ee-8b1f-998d-bed2869297a0@linux.alibaba.com>
Date:   Wed, 8 Sep 2021 17:05:09 +0800
From:   Joseph Qi <joseph.qi@...ux.alibaba.com>
To:     Chenyuan Mi <cymi20@...an.edu.cn>
Cc:     yuanxzhang@...an.edu.cn, Xiyu Yang <xiyuyang19@...an.edu.cn>,
        Xin Tan <tanxin.ctf@...il.com>, Mark Fasheh <mark@...heh.com>,
        Joel Becker <jlbec@...lplan.org>, ocfs2-devel@....oracle.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ocfs2: Fix handle refcount leak in two exception handling
 paths

Hi Chenyuan,
Thanks for reporting this bug.
But the fix looks incorrect. It will commit transaction once more in
normal case.
The simplest way is calling ocfs2_commit_trans() in each of the error
handling path before goto bail.

if (status < 0) {
	ocfs2_commit_trans(osb, handle);
	mlog_errno(status);
	goto bail;
}

Thanks,
Joseph

On 9/8/21 2:26 PM, Chenyuan Mi wrote:
> The reference counting issue happens in two exception handling 
> paths of ocfs2_replay_truncate_records(). When executing these 
> two exception handling paths, the function forgets to decrease 
> the refcount of handle increased by ocfs2_start_trans(), causing 
> a refcount leak.
> 
> Fix this issue by using ocfs2_commit_trans() to decrease the 
> refcount of handle in two handling paths.
> 
> Signed-off-by: Chenyuan Mi <cymi20@...an.edu.cn>
> Signed-off-by: Xiyu Yang <xiyuyang19@...an.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@...il.com>
> 
> ---
>  fs/ocfs2/alloc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index f1cc8258d34a..b87960cdda0d 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -5941,7 +5941,7 @@ static int ocfs2_replay_truncate_records(struct ocfs2_super *osb,
>  						 OCFS2_JOURNAL_ACCESS_WRITE);
>  		if (status < 0) {
>  			mlog_errno(status);
> -			goto bail;
> +			goto bail_commit;
>  		}
>  
>  		tl->tl_used = cpu_to_le16(i);
> @@ -5965,7 +5965,7 @@ static int ocfs2_replay_truncate_records(struct ocfs2_super *osb,
>  						     num_clusters);
>  			if (status < 0) {
>  				mlog_errno(status);
> -				goto bail;
> +				goto bail_commit;
>  			}
>  		}
>  
> @@ -5975,6 +5975,8 @@ static int ocfs2_replay_truncate_records(struct ocfs2_super *osb,
>  
>  	osb->truncated_clusters = 0;
>  
> +bail_commit:
> +	ocfs2_commit_trans(osb, handle);
>  bail:
>  	return status;
>  }
>