linux-kernel - Re: [PATCH linux-next] mm/folio-compat.c: folio should not be NULL when it is referenced

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>] [day] [month] [year] [list]

Date:   Mon, 23 Aug 2021 02:05:15 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Yi Wang <wang.yi59@....com.cn>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, xue.zhihong@....com.cn,
        jiang.xuexin@....com.cn, zealci@....com.cn,
        Changcheng Deng <deng.changcheng@....com.cn>
Subject: Re: [PATCH linux-next] mm/folio-compat.c: folio should not be NULL
 when it is referenced

On Mon, Aug 23, 2021 at 08:47:35AM +0800, Yi Wang wrote:
> From: Changcheng Deng <deng.changcheng@....com.cn>
> 
> A bug was found by coccinelle:
> folio is NULL but dereferenced
> Therefore,added a check to make sure 'folio' is not NULL.
> 
> Reported-by: Zeal Robot <zealci@....com.cn>

Your robot is overzealous.  This does not dereference folio; rather it
takes the address of the page element of the folio structure.

By a strict reading of the C spec, it is not allowed.  However, GCC
(and I assume Clang) does the right thing.

>  	folio = __filemap_get_folio(mapping, index, fgp_flags, gfp);
> -	if ((fgp_flags & FGP_HEAD) || !folio || xa_is_value(folio))
> -		return &folio->page;
> +	if ((fgp_flags & FGP_HEAD) || xa_is_value(folio))
> +		if (folio != NULL)
> +			return &folio->page;
>  	return folio_file_page(folio, index);

This is definitely wrong.  Did you test it?  I bet you get a NULL
pointer dereference if you try it.

You could potentially make the case for:

	if (!folio)
		return NULL;
	if ((fgp_flags & FGP_HEAD) || xa_is_value(folio))
		return &folio->page;

but you actually have the same problem with the C spec, that unless
folio is actually a pointer to a folio, then &folio->page is
_technically_ undefined.  So it would have to be something even
more complex to be pedantically correct.

It's just not worth it.  Fix your tool.