diff mbox series

[1/5] fs: ext4: initialize fsdata in pagecache_write()

Message ID 20221121112134.407362-1-glider@google.com
State New
Headers
Series [1/5] fs: ext4: initialize fsdata in pagecache_write() |

Commit Message

Alexander Potapenko Nov. 21, 2022, 11:21 a.m. UTC 
  When aops->write_begin() does not initialize fsdata, KMSAN reports
an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Cc: Eric Biggers <ebiggers@kernel.org>
Fixes: c93d8f885809 ("ext4: add basic fs-verity support")
Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 fs/ext4/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Morton Nov. 21, 2022, 7:48 p.m. UTC | #1 
On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <glider@google.com> wrote:

> When aops->write_begin() does not initialize fsdata, KMSAN reports
> an error passing the latter to aops->write_end().
> 
> Fix this by unconditionally initializing fsdata.
> 
> ...
>

I'm assuming that this is not-a-bug, and that these changes are purely
workarounds for a KMSAN shortcoming?

If true, this important info should be included in each changelog,
please.

If false, please provide a full description of the end-user visible
effects of the bug.

Also, it would be helpful to describe why it is not considered
practical to teach KMSAN to handle this?

> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
>  		size_t n = min_t(size_t, count,
>  				 PAGE_SIZE - offset_in_page(pos));
>  		struct page *page;
> -		void *fsdata;
> +		void *fsdata = NULL;
>  		int res;
>  
>  		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Eric Biggers Nov. 21, 2022, 7:52 p.m. UTC | #2 
On Mon, Nov 21, 2022 at 12:21:30PM +0100, Alexander Potapenko wrote:
> When aops->write_begin() does not initialize fsdata, KMSAN reports
> an error passing the latter to aops->write_end().
> 
> Fix this by unconditionally initializing fsdata.
> 
> Cc: Eric Biggers <ebiggers@kernel.org>
> Fixes: c93d8f885809 ("ext4: add basic fs-verity support")
> Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  fs/ext4/verity.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> index 3c640bd7ecaeb..30e3b65798b50 100644
> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
>  		size_t n = min_t(size_t, count,
>  				 PAGE_SIZE - offset_in_page(pos));
>  		struct page *page;
> -		void *fsdata;
> +		void *fsdata = NULL;
>  		int res;
>  
>  		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric
Matthew Wilcox Nov. 22, 2022, 3:40 a.m. UTC | #3 
On Mon, Nov 21, 2022 at 11:48:40AM -0800, Andrew Morton wrote:
> On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <glider@google.com> wrote:
> 
> > When aops->write_begin() does not initialize fsdata, KMSAN reports
> > an error passing the latter to aops->write_end().
> > 
> > Fix this by unconditionally initializing fsdata.
> > 
> > ...
> >
> 
> I'm assuming that this is not-a-bug, and that these changes are purely
> workarounds for a KMSAN shortcoming?

It's a weird one.  It used to be not-a-bug.  Then we changed from
std=gnu99 to std=gnu11 or something.  And in the intervening years,
the C standards ctte decided that passing an uninitialised pointer to a
function was UB.  So we start by passing a pointer to the pointer to
->write_begin().  Some ->write_begin functions initialise that pointer;
others don't.  Then we pass the pointer directly to ->write_end.  If
->write_begin initialised the pointer, that's fine, and if not, it's UB.
Of course the ->write_end doesn't use it if the ->write_begin didn't
initialise it, but it's too late because merely calling the function
was UB.  Thanks, Itanium!
diff mbox series

Patch

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index 3c640bd7ecaeb..30e3b65798b50 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -79,7 +79,7 @@  static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
 
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);