Skip to contentSkip to conversation

mm: fix filler_t callback type mismatch with readpage

PATCH In review Sami Tolvanen 3 participants 3 messages Apr 30, 2019 – May 1, 2019

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mm: fix filler_t callback type mismatch with readpage

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.

---
 include/linux/pagemap.h | 22 +++++++++++++++++++---
 mm/filemap.c            |  7 +++++--
 2 files changed, 24 insertions(+), 5 deletions(-)
include/linux/pagemap.h +19 -3
index bcf909d0de5f8..e5652a5ba1584 100644
@@ -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);
 }
 
 /*
mm/filemap.c +5 -2
index d78f577baef2a..6cc41c25ca3bf 100644
@@ -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);
 
Signature · 1 line
2.21.0.593.g511ec345e18-goog
Reviewed-by on v1 1/1
#1 lore raw
Quoted 13 lines · 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

Quoted 7 lines

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.

-Kees

Quoted 62 lines
---
 include/linux/pagemap.h | 22 +++++++++++++++++++---
 mm/filemap.c            |  7 +++++--
 2 files changed, 24 insertions(+), 5 deletions(-)
include/linux/pagemap.h +19 -3
index bcf909d0de5f8..e5652a5ba1584 100644
@@ -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);
 }
 
 /*
mm/filemap.c +5 -3
index d78f577baef2a..6cc41c25ca3bf 100644
@@ -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

Signature · 1 line
Kees Cook