| Message ID | 20190430214724.66699-1-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: fix filler_t callback type mismatch with readpage | expand |
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 >
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
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);
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(-)