Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 4 Jan 2021 15:16:07 -0700
From:   Nathan Chancellor <natechancellor@gmail.com>
To:     trix@redhat.com
Cc:     dwmw2@infradead.org, richard@nod.at, ndesaulniers@google.com,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com
Subject: Re: [PATCH] jffs2: fix use after free in jffs2_sum_write_data()
Message-ID: <20210104221607.GA1985645@ubuntu-m3-large-x86>
References: <20201230145604.1586486-1-trix@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201230145604.1586486-1-trix@redhat.com>
Precedence: bulk

On Wed, Dec 30, 2020 at 06:56:04AM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> clang static analysis reports this problem
> 
> fs/jffs2/summary.c:794:31: warning: Use of memory after it is freed
>                 c->summary->sum_list_head = temp->u.next;
>                                             ^~~~~~~~~~~~
> 
> In jffs2_sum_write_data(), in a loop summary data is handles a node at
> a time.  When it has written out the node it is removed the summary list,
> and the node is deleted.  In the corner case when a
> JFFS2_FEATURE_RWCOMPAT_COPY is seen, a call is made to
> jffs2_sum_disable_collecting().  jffs2_sum_disable_collecting() deletes
> the whole list which conflicts with the loop's deleting the list by parts.
> 
> To preserve the old behavior of stopping the write midway, bail out of
> the loop after disabling summary collection.
> 
> Fixes: 6171586a7ae5 ("[JFFS2] Correct handling of JFFS2_FEATURE_RWCOMPAT_COPY nodes.")
> Signed-off-by: Tom Rix <trix@redhat.com>

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> ---
>  fs/jffs2/summary.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/jffs2/summary.c b/fs/jffs2/summary.c
> index be7c8a6a5748..4fe64519870f 100644
> --- a/fs/jffs2/summary.c
> +++ b/fs/jffs2/summary.c
> @@ -783,6 +783,8 @@ static int jffs2_sum_write_data(struct jffs2_sb_info *c, struct jffs2_eraseblock
>  					dbg_summary("Writing unknown RWCOMPAT_COPY node type %x\n",
>  						    je16_to_cpu(temp->u.nodetype));
>  					jffs2_sum_disable_collecting(c->summary);
> +					/* The above call removes the list, nothing more to do */
> +					goto bail_rwcompat;
>  				} else {
>  					BUG();	/* unknown node in summary information */
>  				}
> @@ -794,6 +796,7 @@ static int jffs2_sum_write_data(struct jffs2_sb_info *c, struct jffs2_eraseblock
>  
>  		c->summary->sum_num--;
>  	}
> + bail_rwcompat:
>  
>  	jffs2_sum_reset_collected(c->summary);
>  
> -- 
> 2.27.0
>