linux-kernel - Re: [PATCH 1/1] fs/ext2: Avoid page_address on pages returned by ext2_get

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: <20210713163018.GF24271@quack2.suse.cz>
Date:   Tue, 13 Jul 2021 18:30:18 +0200
From:   Jan Kara <jack@...e.cz>
To:     Javier Pello <javier.pello@...c.es>
Cc:     Jan Kara <jack@...e.com>, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org, Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH 1/1] fs/ext2: Avoid page_address on pages returned by
 ext2_get_page

On Tue 13-07-21 16:59:18, Javier Pello wrote:
> From: Javier Pello <javier.pello@...c.es>
> 
> Commit 782b76d7abdf02b12c46ed6f1e9bf715569027f7 ("fs/ext2: Replace
> kmap() with kmap_local_page()") replaced the kmap/kunmap calls in
> ext2_get_page/ext2_put_page with kmap_local_page/kunmap_local for
> efficiency reasons. As a necessary side change, the commit also
> made ext2_get_page (and ext2_find_entry and ext2_dotdot) return
> the mapping address along with the page itself, as it is required
> for kunmap_local, and converted uses of page_address on such pages
> to use the newly returned address instead. However, uses of
> page_address on such pages were missed in ext2_check_page and
> ext2_delete_entry, which triggers oopses if kmap_local_page happens
> to return an address from high memory. Fix this now by converting
> the remaining uses of page_address to use the right address, as
> returned by kmap_local_page.

Good catch. Thanks for the patch. Ira, can you please check the patch as
well?

I have just a few nits below:

> Signed-off-by: Javier Pello <javier.pello@...c.es>
> Fixes: 782b76d7abdf fs/ext2: Replace kmap() with kmap_local_page()

Please wrap subject in Fixes tag into ("...").

> @@ -584,16 +584,16 @@ int ext2_add_link (struct dentry *dentry, struct inode *inode)
>   * ext2_delete_entry deletes a directory entry by merging it with the
>   * previous entry. Page is up-to-date.
>   */
> -int ext2_delete_entry (struct ext2_dir_entry_2 * dir, struct page * page )
> +int ext2_delete_entry (struct ext2_dir_entry_2 *dir, struct page *page,
> +			void *kaddr)

Why not have 'kaddr' as char *. We type it to char * basically everywhere
anyway.

>  {
>  	struct inode *inode = page->mapping->host;
> -	char *kaddr = page_address(page);
> -	unsigned from = ((char*)dir - kaddr) & ~(ext2_chunk_size(inode)-1);
> -	unsigned to = ((char *)dir - kaddr) +
> -				ext2_rec_len_from_disk(dir->rec_len);
> +	unsigned int delta = (char *)dir - (char *)kaddr;

Maybe I'd call this 'offset' rather than 'delta'. Also if kaddr will stay
char *, you maybe just don't need to touch this...

> +	unsigned int from = delta & ~(ext2_chunk_size(inode)-1);
> +	unsigned int to = delta + ext2_rec_len_from_disk(dir->rec_len);
>  	loff_t pos;
>  	ext2_dirent * pde = NULL;
> -	ext2_dirent * de = (ext2_dirent *) (kaddr + from);
> +	ext2_dirent *de = (ext2_dirent *) ((char *)kaddr + from);
>  	int err;
>  
>  	while ((char*)de < (char*)dir) {
> @@ -607,7 +607,7 @@ int ext2_delete_entry (struct ext2_dir_entry_2 * dir, struct page * page )
>  		de = ext2_next_entry(de);
>  	}
>  	if (pde)
> -		from = (char*)pde - (char*)page_address(page);
> +		from = (char *)pde - (char *)kaddr;
>  	pos = page_offset(page) + from;
>  	lock_page(page);
>  	err = ext2_prepare_chunk(page, pos, to - from);

									Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR