Casting mapping->a_ops->readpage to filler_t causes an indirect call
type mismatch with Control-Flow Integrity checking. This change fixes
the mismatch in read_cache_page_gfp and read_mapping_page by adding
using a NULL filler argument as an indication to call ->readpage
directly, and by passing the right parameter callbacks in nfs and jffs2.
We can just pass a NULL filler and do the right thing inside of
do_read_cache_page based on the NULL parameter.
Signed-off-by: Christoph Hellwig <[email protected]>
---
include/linux/pagemap.h | 3 +--
mm/filemap.c | 10 ++++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index bcf909d0de5f..f52c3a2074cd 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -386,8 +386,7 @@ extern int read_cache_pages(struct address_space *mapping,
static inline struct page *read_mapping_page(struct address_space *mapping,
pgoff_t index, void *data)
{
- filler_t *filler = (filler_t *)mapping->a_ops->readpage;
- return read_cache_page(mapping, index, filler, data);
+ return read_cache_page(mapping, index, NULL, data);
}
/*
diff --git a/mm/filemap.c b/mm/filemap.c
index a2fc59f56f50..51f5b02c299a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2866,7 +2866,11 @@ static struct page *do_read_cache_page(struct address_space *mapping,
}
filler:
- err = filler(data, page);
+ if (filler)
+ err = filler(data, page);
+ else
+ err = mapping->a_ops->readpage(data, page);
+
if (err < 0) {
put_page(page);
return ERR_PTR(err);
@@ -2978,9 +2982,7 @@ struct page *read_cache_page_gfp(struct address_space *mapping,
pgoff_t index,
gfp_t gfp)
{
- filler_t *filler = (filler_t *)mapping->a_ops->readpage;
-
- return do_read_cache_page(mapping, index, filler, NULL, gfp);
+ return do_read_cache_page(mapping, index, NULL, NULL, gfp);
}
EXPORT_SYMBOL(read_cache_page_gfp);
--
2.20.1
Fix the callback jffs2 passes to read_cache_page to actually have the
proper type expected. Casting around function pointers can easily
hide typing bugs, and defeats control flow protection.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/jffs2/file.c | 4 ++--
fs/jffs2/fs.c | 2 +-
fs/jffs2/os-linux.h | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index 7d8654a1472e..f8fb89b10227 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -109,9 +109,9 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
return ret;
}
-int jffs2_do_readpage_unlock(struct inode *inode, struct page *pg)
+int jffs2_do_readpage_unlock(void *data, struct page *pg)
{
- int ret = jffs2_do_readpage_nolock(inode, pg);
+ int ret = jffs2_do_readpage_nolock(data, pg);
unlock_page(pg);
return ret;
}
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index eab04eca95a3..7fbe8a7843b9 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -686,7 +686,7 @@ unsigned char *jffs2_gc_fetch_page(struct jffs2_sb_info *c,
struct page *pg;
pg = read_cache_page(inode->i_mapping, offset >> PAGE_SHIFT,
- (void *)jffs2_do_readpage_unlock, inode);
+ jffs2_do_readpage_unlock, inode);
if (IS_ERR(pg))
return (void *)pg;
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index a2dbbb3f4c74..bd3d5f0ddc34 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -155,7 +155,7 @@ extern const struct file_operations jffs2_file_operations;
extern const struct inode_operations jffs2_file_inode_operations;
extern const struct address_space_operations jffs2_file_address_operations;
int jffs2_fsync(struct file *, loff_t, loff_t, int);
-int jffs2_do_readpage_unlock (struct inode *inode, struct page *pg);
+int jffs2_do_readpage_unlock(void *data, struct page *pg);
/* ioctl.c */
long jffs2_ioctl(struct file *, unsigned int, unsigned long);
--
2.20.1
Fix the callbacks NFS passes to read_cache_page to actually have the
proper type expected. Casting around function pointers can easily
hide typing bugs, and defeats control flow protection.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/nfs/dir.c | 7 ++++---
fs/nfs/symlink.c | 7 ++++---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index a71d0b42d160..47d445bec8c9 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -714,8 +714,9 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
* We only need to convert from xdr once so future lookups are much simpler
*/
static
-int nfs_readdir_filler(nfs_readdir_descriptor_t *desc, struct page* page)
+int nfs_readdir_filler(void *data, struct page* page)
{
+ nfs_readdir_descriptor_t *desc = data;
struct inode *inode = file_inode(desc->file);
int ret;
@@ -762,8 +763,8 @@ void cache_page_release(nfs_readdir_descriptor_t *desc)
static
struct page *get_cache_page(nfs_readdir_descriptor_t *desc)
{
- return read_cache_page(desc->file->f_mapping,
- desc->page_index, (filler_t *)nfs_readdir_filler, desc);
+ return read_cache_page(desc->file->f_mapping, desc->page_index,
+ nfs_readdir_filler, desc);
}
/*
diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
index 06eb44b47885..25ba299fdac2 100644
--- a/fs/nfs/symlink.c
+++ b/fs/nfs/symlink.c
@@ -26,8 +26,9 @@
* and straight-forward than readdir caching.
*/
-static int nfs_symlink_filler(struct inode *inode, struct page *page)
+static int nfs_symlink_filler(void *data, struct page *page)
{
+ struct inode *inode = data;
int error;
error = NFS_PROTO(inode)->readlink(inode, page, 0, PAGE_SIZE);
@@ -65,8 +66,8 @@ static const char *nfs_get_link(struct dentry *dentry,
err = ERR_PTR(nfs_revalidate_mapping(inode, inode->i_mapping));
if (err)
return err;
- page = read_cache_page(&inode->i_data, 0,
- (filler_t *)nfs_symlink_filler, inode);
+ page = read_cache_page(&inode->i_data, 0, nfs_symlink_filler,
+ inode);
if (IS_ERR(page))
return ERR_CAST(page);
}
--
2.20.1
Signed-off-by: Christoph Hellwig <[email protected]>
---
mm/filemap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index d78f577baef2..a2fc59f56f50 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2956,7 +2956,8 @@ struct page *read_cache_page(struct address_space *mapping,
int (*filler)(void *, struct page *),
void *data)
{
- return do_read_cache_page(mapping, index, filler, data, mapping_gfp_mask(mapping));
+ return do_read_cache_page(mapping, index, filler, data,
+ mapping_gfp_mask(mapping));
}
EXPORT_SYMBOL(read_cache_page);
--
2.20.1
On Wed, May 1, 2019 at 9:07 AM Christoph Hellwig <[email protected]> wrote:
>
> Casting mapping->a_ops->readpage to filler_t causes an indirect call
> type mismatch with Control-Flow Integrity checking. This change fixes
> the mismatch in read_cache_page_gfp and read_mapping_page by adding
> using a NULL filler argument as an indication to call ->readpage
> directly, and by passing the right parameter callbacks in nfs and jffs2.
>
Thanks, Christoph! This looks much cleaner.
I tested the patches on a kernel compiled with clang's -fsanitize=cfi
and the fixes look good to me. However, you missed one more type
mismatch in v9fs_vfs_readpages (fs/9p/vfs_addr.c). Could you please
add that one to the series too?
Sami
Fix the callback 9p passes to read_cache_page to actually have the
proper type expected. Casting around function pointers can easily
hide typing bugs, and defeats control flow protection.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/9p/vfs_addr.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
index 0bcbcc20f769..02e0fc51401e 100644
--- a/fs/9p/vfs_addr.c
+++ b/fs/9p/vfs_addr.c
@@ -50,8 +50,9 @@
* @page: structure to page
*
*/
-static int v9fs_fid_readpage(struct p9_fid *fid, struct page *page)
+static int v9fs_fid_readpage(void *data, struct page *page)
{
+ struct p9_fid *fid = data;
struct inode *inode = page->mapping->host;
struct bio_vec bvec = {.bv_page = page, .bv_len = PAGE_SIZE};
struct iov_iter to;
@@ -122,7 +123,8 @@ static int v9fs_vfs_readpages(struct file *filp, struct address_space *mapping,
if (ret == 0)
return ret;
- ret = read_cache_pages(mapping, pages, (void *)v9fs_vfs_readpage, filp);
+ ret = read_cache_pages(mapping, pages, v9fs_fid_readpage,
+ filp->private_data);
p9_debug(P9_DEBUG_VFS, " = %d\n", ret);
return ret;
}
--
2.20.1
1) You need to pass "filp" rather than "filp->private_data" to read_cache_pages()
in v9fs_fid_readpage().
The patched code passes "filp->private_data" as the "data" parameter to
read_cache_pages(), which would generate a call to:
filler(data, page)
which would become a call to:
static int v9fs_vfs_readpage(struct file *filp, struct page *page)
{
return v9fs_fid_readpage(filp->private_data, page);
}
which would then effectively become:
v9fs_fid_readpage(filp->private_data->private_data, page)
Which isn't correct; because data is a void *, no error is thrown when
v9fs_vfs_readpages treats filp->private_data as if it is filp.
2) I'd also like to see an explicit comment in do_read_cache_page() along
the lines of:
/*
* If a custom page filler was passed in use it, otherwise use the
* standard readpage() routine defined for the address_space.
*
*/
3) Patch 5/4?
Otherwise it looks good.
Reviewed-by: William Kucharski <[email protected]>
> On May 1, 2019, at 11:34 AM, Christoph Hellwig <[email protected]> wrote:
>
> Fix the callback 9p passes to read_cache_page to actually have the
> proper type expected. Casting around function pointers can easily
> hide typing bugs, and defeats control flow protection.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/9p/vfs_addr.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
> index 0bcbcc20f769..02e0fc51401e 100644
> --- a/fs/9p/vfs_addr.c
> +++ b/fs/9p/vfs_addr.c
> @@ -50,8 +50,9 @@
> * @page: structure to page
> *
> */
> -static int v9fs_fid_readpage(struct p9_fid *fid, struct page *page)
> +static int v9fs_fid_readpage(void *data, struct page *page)
> {
> + struct p9_fid *fid = data;
> struct inode *inode = page->mapping->host;
> struct bio_vec bvec = {.bv_page = page, .bv_len = PAGE_SIZE};
> struct iov_iter to;
> @@ -122,7 +123,8 @@ static int v9fs_vfs_readpages(struct file *filp, struct address_space *mapping,
> if (ret == 0)
> return ret;
>
> - ret = read_cache_pages(mapping, pages, (void *)v9fs_vfs_readpage, filp);
> + ret = read_cache_pages(mapping, pages, v9fs_fid_readpage,
> + filp->private_data);
> p9_debug(P9_DEBUG_VFS, " = %d\n", ret);
> return ret;
> }
On Thu, May 02, 2019 at 12:08:29AM -0600, William Kucharski wrote:
> 3) Patch 5/4?
That's a relatively common notation when an extra patch is needed to fix
something after a series has been sent ;-)
> On May 2, 2019, at 7:04 AM, Christoph Hellwig <[email protected]> wrote:
>
> Except that we don't pass v9fs_vfs_readpage as the filler any more,
> we now pass v9fs_fid_readpage.
True, so never mind. :-)