[PATCH] mm: fix filler_t callback type mismatch with readpage

[PATCH] mm: fix filler_t callback type mismatch with readpage

[Sami Tolvanen]

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 a
stub callback function with the correct type.

As the kernel only has a couple of instances of read_cache_page(s)
being called with a callback function that doesn't accept struct file*
as the first parameter, Android kernels have previously fixed this by
changing filler_t to int (*filler_t)(struct file *, struct page *):

  https://android-review.googlesource.com/c/kernel/common/+/671260

While this approach did fix most of the issues, the few remaining
cases where unrelated private data are passed to the callback become
rather awkward. Keeping filler_t unchanged and using a stub function
for readpage instead solves this problem.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/linux/pagemap.h | 22 +++++++++++++++++++---
 mm/filemap.c            |  7 +++++--
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index bcf909d0de5f8..e5652a5ba1584 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -383,11 +383,27 @@ extern struct page * read_cache_page_gfp(struct address_space *mapping,
 extern int read_cache_pages(struct address_space *mapping,
 		struct list_head *pages, filler_t *filler, void *data);
 
+struct file_filler_data {
+	int (*filler)(struct file *, struct page *);
+	struct file *filp;
+};
+
+static inline int __file_filler(void *data, struct page *page)
+{
+	struct file_filler_data *ffd = (struct file_filler_data *)data;
+
+	return ffd->filler(ffd->filp, page);
+}
+
 static inline struct page *read_mapping_page(struct address_space *mapping,
-				pgoff_t index, void *data)
+				pgoff_t index, struct file *filp)
 {
-	filler_t *filler = (filler_t *)mapping->a_ops->readpage;
-	return read_cache_page(mapping, index, filler, data);
+	struct file_filler_data data = {
+		.filler = mapping->a_ops->readpage,
+		.filp   = filp
+	};
+
+	return read_cache_page(mapping, index, __file_filler, &data);
 }
 
 /*
diff --git a/mm/filemap.c b/mm/filemap.c
index d78f577baef2a..6cc41c25ca3bf 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2977,9 +2977,12 @@ 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;
+	struct file_filler_data data = {
+		.filler = mapping->a_ops->readpage,
+		.filp	= NULL
+	};
 
-	return do_read_cache_page(mapping, index, filler, NULL, gfp);
+	return do_read_cache_page(mapping, index, __file_filler, &data, gfp);
 }
 EXPORT_SYMBOL(read_cache_page_gfp);
 
-- 
2.21.0.593.g511ec345e18-goog

Re: [PATCH] mm: fix filler_t callback type mismatch with readpage

[Kees Cook]

On Tue, Apr 30, 2019 at 2:47 PM Sami Tolvanen <samitolvanen@google.com> 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 a
> stub callback function with the correct type.
>
> As the kernel only has a couple of instances of read_cache_page(s)
> being called with a callback function that doesn't accept struct file*
> as the first parameter, Android kernels have previously fixed this by
> changing filler_t to int (*filler_t)(struct file *, struct page *):
>
>   https://android-review.googlesource.com/c/kernel/common/+/671260
>
> While this approach did fix most of the issues, the few remaining
> cases where unrelated private data are passed to the callback become
> rather awkward. Keeping filler_t unchanged and using a stub function
> for readpage instead solves this problem.
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  include/linux/pagemap.h | 22 +++++++++++++++++++---
>  mm/filemap.c            |  7 +++++--
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index bcf909d0de5f8..e5652a5ba1584 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -383,11 +383,27 @@ extern struct page * read_cache_page_gfp(struct address_space *mapping,
>  extern int read_cache_pages(struct address_space *mapping,
>                 struct list_head *pages, filler_t *filler, void *data);
>
> +struct file_filler_data {
> +       int (*filler)(struct file *, struct page *);
> +       struct file *filp;
> +};
> +
> +static inline int __file_filler(void *data, struct page *page)
> +{
> +       struct file_filler_data *ffd = (struct file_filler_data *)data;
> +
> +       return ffd->filler(ffd->filp, page);
> +}
> +
>  static inline struct page *read_mapping_page(struct address_space *mapping,
> -                               pgoff_t index, void *data)
> +                               pgoff_t index, struct file *filp)
>  {
> -       filler_t *filler = (filler_t *)mapping->a_ops->readpage;
> -       return read_cache_page(mapping, index, filler, data);
> +       struct file_filler_data data = {
> +               .filler = mapping->a_ops->readpage,
> +               .filp   = filp
> +       };
> +
> +       return read_cache_page(mapping, index, __file_filler, &data);
>  }
>
>  /*
> diff --git a/mm/filemap.c b/mm/filemap.c
> index d78f577baef2a..6cc41c25ca3bf 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2977,9 +2977,12 @@ 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;
> +       struct file_filler_data data = {
> +               .filler = mapping->a_ops->readpage,
> +               .filp   = NULL
> +       };
>
> -       return do_read_cache_page(mapping, index, filler, NULL, gfp);
> +       return do_read_cache_page(mapping, index, __file_filler, &data, gfp);
>  }
>  EXPORT_SYMBOL(read_cache_page_gfp);
>
> --
> 2.21.0.593.g511ec345e18-goog
>


-- 
Kees Cook

Re: [PATCH] mm: fix filler_t callback type mismatch with readpage

[Christoph Hellwig]

This still leaves bugs around in jffs2 and nfs.  And it is a little
ugly.  This is what I'd like to do instead, so far untested.  I'll
post a series once it passes basic testing:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/filler-fixes