[PATCH] fs: buffer: Check to avoid NULL pointer dereference of returned buffer_head for a private page
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);
/*
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>
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
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
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?
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 )
linux-stable-mirror@lists.linaro.org
-
Al Viro -
Greg KH -
Monthero Ronald -
Ron Enfield