linux-kernel - Re: [PATCH 3/3] fs/ufs: Replace kmap() with kmap_local

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

Message-ID: <Y5ZcMPzPG9h6C9eh@ZenIV>
Date:   Sun, 11 Dec 2022 22:39:44 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Evgeniy Dushistov <dushistov@...l.ru>,
        Ira Weiny <ira.weiny@...el.com>, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 3/3] fs/ufs: Replace kmap() with kmap_local_page()

On Sun, Dec 11, 2022 at 10:31:11PM +0100, Fabio M. De Francesco wrote:

> +/*
> + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> + * rules documented in kmap_local_page()/kunmap_local().
> + *
> + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page()
> + * and must be treated accordingly for nesting purposes.
> + */
>  static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **page)
>  {
> +	char *kaddr;
> +
>  	struct address_space *mapping = dir->i_mapping;
>  	*page = read_mapping_page(mapping, n, NULL);
>  	if (!IS_ERR(*page)) {
> -		kmap(*page);
> +		kmap_local_page(*page);
>  		if (unlikely(!PageChecked(*page))) {
> -			if (!ufs_check_page(*page))
> +			if (!ufs_check_page(*page, kaddr))

	Er...  Building the patched tree is occasionally useful.
Here kaddr is obviously uninitialized and compiler would've
probably caught that.

	And return value of kmap_local_page() is lost, which
is related to the previous issue ;-)


>  				goto fail;
>  		}
>  	}
> -	return page;
> +	return *page;

Hell, no.  Callers expect the pointer to the first byte of
your page.  What it should return is kaddr.

> @@ -388,7 +406,8 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
>  	mark_inode_dirty(dir);
>  	/* OFFSET_CACHE */
>  out_put:
> -	ufs_put_page(page);
> +	ufs_put_page(page, kaddr);
> +	return 0;
>  out_unlock:
>  	unlock_page(page);
>  	goto out_put;

That can't be right.  Places like
        if (err)
		goto out_unlock;
do not expect err to be lost.  You end up returning 0 now.  Something strange
happened here (in the previous commit, perhaps?)