[PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[Monthero Ronald]

The patch checks for this condition of NULL pointer for the buffer_head returned from page_buffers()
and also a check placed within the list traversal loop for next buffer_head structs.

crash scenario:
The buffer_head returned from page_buffers() is not checked in block_invalidatepage_range function.
The struct buffer_head* pointer returned by page_buffers(page) was 0x0, although this page had its
private flag PG_private bit set and was expected to have buffer_head structs attached.The NULL pointer
buffer_head was dereferenced in block_invalidatepage_range function at bh->b_size, where bh returned by
page_buffers(page) was 0x0.

The stack frames were  truncate_inode_page() => do_invalidatepage_range() => xfs_vm_invalidatepage() =>
          [exception RIP: block_invalidatepage_range+132]

The inode for truncate in this case was valid and had  proper inode.i_state = 0x20 - FREEING and had
a valid mapped address space to xfs. And the struct page in context of block_invalidatepage_range()
had its page flag PG_private set but the page.private was 0x0. So page_buffers(page) returned 0x0
and hence the crash.
This patch performs NULL pointer check for returned buffer_head. Applies to 3.16 and later kernels.

Signed-off-by: Monthero Ronald <rhmcruiser@gmail.com>
---
 fs/buffer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index eba6e4f..fa80cf4 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1541,6 +1541,7 @@ void block_invalidatepage(struct page *page, unsigned int offset,
 	BUG_ON(stop > PAGE_CACHE_SIZE || stop < length);
 
 	head = page_buffers(page);
+	BUG_ON(!head);
 	bh = head;
 	do {
 		unsigned int next_off = curr_off + bh->b_size;
@@ -1559,6 +1560,7 @@ void block_invalidatepage(struct page *page, unsigned int offset,
 			discard_buffer(bh);
 		curr_off = next_off;
 		bh = next;
+		BUG_ON(!bh);
 	} while (bh != head);
 
 	/*
-- 
1.8.3.1

Re: [PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[Greg KH]

On Thu, Aug 15, 2019 at 01:09:11AM +1000, Monthero Ronald wrote:
> The patch checks for this condition of NULL pointer for the buffer_head returned from page_buffers()
> and also a check placed within the list traversal loop for next buffer_head structs.
> 
> crash scenario:
> The buffer_head returned from page_buffers() is not checked in block_invalidatepage_range function.
> The struct buffer_head* pointer returned by page_buffers(page) was 0x0, although this page had its
> private flag PG_private bit set and was expected to have buffer_head structs attached.The NULL pointer
> buffer_head was dereferenced in block_invalidatepage_range function at bh->b_size, where bh returned by
> page_buffers(page) was 0x0.
> 
> The stack frames were  truncate_inode_page() => do_invalidatepage_range() => xfs_vm_invalidatepage() =>
>           [exception RIP: block_invalidatepage_range+132]
> 
> The inode for truncate in this case was valid and had  proper inode.i_state = 0x20 - FREEING and had
> a valid mapped address space to xfs. And the struct page in context of block_invalidatepage_range()
> had its page flag PG_private set but the page.private was 0x0. So page_buffers(page) returned 0x0
> and hence the crash.
> This patch performs NULL pointer check for returned buffer_head. Applies to 3.16 and later kernels.
> 
> Signed-off-by: Monthero Ronald <rhmcruiser@gmail.com>
> ---
>  fs/buffer.c | 2 ++
>  1 file changed, 2 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[Ron Enfield]

On Thu, Aug 15, 2019 at 2:03 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 15, 2019 at 01:09:11AM +1000, Monthero Ronald wrote:
> > The patch checks for this condition of NULL pointer for the buffer_head returned from page_buffers()
> > and also a check placed within the list traversal loop for next buffer_head structs.
> >
> > crash scenario:
> > The buffer_head returned from page_buffers() is not checked in block_invalidatepage_range function.
> > The struct buffer_head* pointer returned by page_buffers(page) was 0x0, although this page had its
> > private flag PG_private bit set and was expected to have buffer_head structs attached.The NULL pointer
> > buffer_head was dereferenced in block_invalidatepage_range function at bh->b_size, where bh returned by
> > page_buffers(page) was 0x0.
> >
> > The stack frames were  truncate_inode_page() => do_invalidatepage_range() => xfs_vm_invalidatepage() =>
> >           [exception RIP: block_invalidatepage_range+132]
> >
> > The inode for truncate in this case was valid and had  proper inode.i_state = 0x20 - FREEING and had
> > a valid mapped address space to xfs. And the struct page in context of block_invalidatepage_range()
> > had its page flag PG_private set but the page.private was 0x0. So page_buffers(page) returned 0x0
> > and hence the crash.
> > This patch performs NULL pointer check for returned buffer_head. Applies to 3.16 and later kernels.
> >
> > Signed-off-by: Monthero Ronald <rhmcruiser@gmail.com>
> > ---
> >  fs/buffer.c | 2 ++
> >  1 file changed, 2 insertions(+)
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>

Hi Greg,
Thanks for feedback . Sorry I might have missed some steps.  I am
checking through the patch list requirements.
I know there could be a couple of issue but  if you or Viro to please clarify.

Please clarify.
- I believe I should only email the maintainer of fs and not email the
list stable@vger.kernel.org at this stage of submission.
  ( I ran  the get-maintainer.pl script it showed Viro as maintainer
for fs/buffer.c , apology if not )
- The patch is around 47 lines including the comments, I assume thats okay.
    ( I have not included the crashed bt, page, buffer_head and inode
details just to keep it short )

- The crash occurred on 3.16 kernel but the buffer.c:
block_invalidatepage_range( ) code path  in later kernels of 4.xx are
also same as this.
   So it may apply to later kernels too.  Please let me know if this
is not the right way to do it.
Thanks for your help and time
Ron

Re: [PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[Greg KH]

On Sun, Aug 18, 2019 at 02:27:29PM +1000, Ron Enfield wrote:
> On Thu, Aug 15, 2019 at 2:03 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 15, 2019 at 01:09:11AM +1000, Monthero Ronald wrote:
> > > The patch checks for this condition of NULL pointer for the buffer_head returned from page_buffers()
> > > and also a check placed within the list traversal loop for next buffer_head structs.
> > >
> > > crash scenario:
> > > The buffer_head returned from page_buffers() is not checked in block_invalidatepage_range function.
> > > The struct buffer_head* pointer returned by page_buffers(page) was 0x0, although this page had its
> > > private flag PG_private bit set and was expected to have buffer_head structs attached.The NULL pointer
> > > buffer_head was dereferenced in block_invalidatepage_range function at bh->b_size, where bh returned by
> > > page_buffers(page) was 0x0.
> > >
> > > The stack frames were  truncate_inode_page() => do_invalidatepage_range() => xfs_vm_invalidatepage() =>
> > >           [exception RIP: block_invalidatepage_range+132]
> > >
> > > The inode for truncate in this case was valid and had  proper inode.i_state = 0x20 - FREEING and had
> > > a valid mapped address space to xfs. And the struct page in context of block_invalidatepage_range()
> > > had its page flag PG_private set but the page.private was 0x0. So page_buffers(page) returned 0x0
> > > and hence the crash.
> > > This patch performs NULL pointer check for returned buffer_head. Applies to 3.16 and later kernels.
> > >
> > > Signed-off-by: Monthero Ronald <rhmcruiser@gmail.com>
> > > ---
> > >  fs/buffer.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> >
> > <formletter>
> >
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> >
> > </formletter>
> 
> Hi Greg,
> Thanks for feedback . Sorry I might have missed some steps.  I am
> checking through the patch list requirements.
> I know there could be a couple of issue but  if you or Viro to please clarify.
> 
> Please clarify.
> - I believe I should only email the maintainer of fs and not email the
> list stable@vger.kernel.org at this stage of submission.

Correct.

>   ( I ran  the get-maintainer.pl script it showed Viro as maintainer
> for fs/buffer.c , apology if not )
> - The patch is around 47 lines including the comments, I assume thats okay.
>     ( I have not included the crashed bt, page, buffer_head and inode
> details just to keep it short )
> 
> - The crash occurred on 3.16 kernel but the buffer.c:
> block_invalidatepage_range( ) code path  in later kernels of 4.xx are
> also same as this.
>    So it may apply to later kernels too.  Please let me know if this
> is not the right way to do it.

You need to test the patch on the latest kernel release.  3.16 is many
many years old.

Also, use the mailing lists that get_maintainer.pl said to use.

thanks,

greg k-h

Re: [PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[Al Viro]

On Thu, Aug 15, 2019 at 01:09:11AM +1000, Monthero Ronald wrote:
> The patch checks for this condition of NULL pointer for the buffer_head returned from page_buffers()
> and also a check placed within the list traversal loop for next buffer_head structs.
> 
> crash scenario:
> The buffer_head returned from page_buffers() is not checked in block_invalidatepage_range function.
> The struct buffer_head* pointer returned by page_buffers(page) was 0x0, although this page had its
> private flag PG_private bit set and was expected to have buffer_head structs attached.The NULL pointer
> buffer_head was dereferenced in block_invalidatepage_range function at bh->b_size, where bh returned by
> page_buffers(page) was 0x0.
> 
> The stack frames were  truncate_inode_page() => do_invalidatepage_range() => xfs_vm_invalidatepage() =>
>           [exception RIP: block_invalidatepage_range+132]
> 
> The inode for truncate in this case was valid and had  proper inode.i_state = 0x20 - FREEING and had
> a valid mapped address space to xfs. And the struct page in context of block_invalidatepage_range()
> had its page flag PG_private set but the page.private was 0x0. So page_buffers(page) returned 0x0
> and hence the crash.
> This patch performs NULL pointer check for returned buffer_head. Applies to 3.16 and later kernels.

... and adds BUG_ON() for that.  The only real difference from an oops is
that it's a bit easier to recognize.  Which may or may not be a good
debugging strategy, but what's the point of having it in -stable?  Or
anywhere other than the build on the boxen you are testing on...

It doesn't fix the underlying bug.  It doesn't tell where the problem
is.  It's definitely *not* a way to fix any bugs.  And while we
are at it, the stuff in -stable ought to be backports from mainline.

Can you reproduce your crashes on mainline?

Re: [PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page

[Ron Enfield]

On Sun, Aug 18, 2019 at 2:31 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Aug 15, 2019 at 01:09:11AM +1000, Monthero Ronald wrote:
> > The patch checks for this condition of NULL pointer for the buffer_head returned from page_buffers()
> > and also a check placed within the list traversal loop for next buffer_head structs.
> >
> > crash scenario:
> > The buffer_head returned from page_buffers() is not checked in block_invalidatepage_range function.
> > The struct buffer_head* pointer returned by page_buffers(page) was 0x0, although this page had its
> > private flag PG_private bit set and was expected to have buffer_head structs attached.The NULL pointer
> > buffer_head was dereferenced in block_invalidatepage_range function at bh->b_size, where bh returned by
> > page_buffers(page) was 0x0.
> >
> > The stack frames were  truncate_inode_page() => do_invalidatepage_range() => xfs_vm_invalidatepage() =>
> >           [exception RIP: block_invalidatepage_range+132]
> >
> > The inode for truncate in this case was valid and had  proper inode.i_state = 0x20 - FREEING and had
> > a valid mapped address space to xfs. And the struct page in context of block_invalidatepage_range()
> > had its page flag PG_private set but the page.private was 0x0. So page_buffers(page) returned 0x0
> > and hence the crash.
> > This patch performs NULL pointer check for returned buffer_head. Applies to 3.16 and later kernels.
>
> ... and adds BUG_ON() for that.  The only real difference from an oops is
> that it's a bit easier to recognize.  Which may or may not be a good
> debugging strategy, but what's the point of having it in -stable?  Or
> anywhere other than the build on the boxen you are testing on...
>
> It doesn't fix the underlying bug.  It doesn't tell where the problem
> is.  It's definitely *not* a way to fix any bugs.  And while we
> are at it, the stuff in -stable ought to be backports from mainline.
>
> Can you reproduce your crashes on mainline?

No I don't have a reproducer.  This was an abrupt crash, where the
page has its private flag set but the buffer_head returned by
page_buffers( ) was 0x0. Having the BUG_ON check for the returned
buffer_head from page_buffers( ) would help to to see the BUG line
readily than to analyze the vmcore  and check the fault condition why
kernel panicked. Two cents here, but open to thoughts.
( This crash to reproduce is not trivial, nevertheless will attempt to
check if there are any mm tests or page mapping tests that can induce
this condition )