2013-10-17 14:49:40

by Laura Abbott

Subject: [PATCH] mm: Check for NULL return values from allocating functions

A security audit revealed that several functions were not checking
return value of allocation functions. These allocations may return
NULL which may lead to NULL pointer dereferences and crashes or
security concerns. Fix this by properly checking the return value
and handling the error appropriately.

Signed-off-by: Laura Abbott <[email protected]>
---
fs/buffer.c | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 4d74335..b53f863 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1561,6 +1561,9 @@ void create_empty_buffers(struct page *page,
struct buffer_head *bh, *head, *tail;

head = alloc_page_buffers(page, blocksize, 1);
+ if (head == NULL)
+ return;
+
bh = head;
do {
bh->b_state |= b_state;
@@ -3008,16 +3011,18 @@ int _submit_bh(int rw, struct buffer_head *bh, unsigned long bio_flags)
BUG_ON(buffer_unwritten(bh));

/*
- * Only clear out a write error when rewriting
- */
- if (test_set_buffer_req(bh) && (rw & WRITE))
- clear_buffer_write_io_error(bh);
-
- /*
* from here on down, it's all bio -- do the initial mapping,
* submit_bio -> generic_make_request may further map this bio around
*/
bio = bio_alloc(GFP_NOIO, 1);
+ if (bio == NULL)
+ return -ENOMEM;
+
+ /*
+ * Only clear out a write error when rewriting
+ */
+ if (test_set_buffer_req(bh) && (rw & WRITE))
+ clear_buffer_write_io_error(bh);

bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
bio->bi_bdev = bh->b_bdev;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2013-10-18 08:40:35

by Jan Kara

Subject: Re: [PATCH] mm: Check for NULL return values from allocating functions

On Thu 17-10-13 07:49:34, Laura Abbott wrote:
> A security audit revealed that several functions were not checking
> return value of allocation functions. These allocations may return
> NULL which may lead to NULL pointer dereferences and crashes or
> security concerns. Fix this by properly checking the return value
> and handling the error appropriately.
>
> Signed-off-by: Laura Abbott <[email protected]>
> ---
> fs/buffer.c | 17 +++++++++++------
> 1 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 4d74335..b53f863 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1561,6 +1561,9 @@ void create_empty_buffers(struct page *page,
> struct buffer_head *bh, *head, *tail;
>
> head = alloc_page_buffers(page, blocksize, 1);
> + if (head == NULL)
> + return;
> +
This cannot happen. alloc_page_buffers() is called with retry == 1 and
thus it will loop until it gets the memory it wants.

> bh = head;
> do {
> bh->b_state |= b_state;
> @@ -3008,16 +3011,18 @@ int _submit_bh(int rw, struct buffer_head *bh, unsigned long bio_flags)
> BUG_ON(buffer_unwritten(bh));
>
> /*
> - * Only clear out a write error when rewriting
> - */
> - if (test_set_buffer_req(bh) && (rw & WRITE))
> - clear_buffer_write_io_error(bh);
> -
> - /*
> * from here on down, it's all bio -- do the initial mapping,
> * submit_bio -> generic_make_request may further map this bio around
> */
> bio = bio_alloc(GFP_NOIO, 1);
> + if (bio == NULL)
> + return -ENOMEM;
And the same is true here. If the gfp mask has __GFP_WAIT set (and
GFP_NOIO does have that), mempool_alloc() loops until it gets the memory.
So I agree we might be missing some details in documentation but the code
is correct.

Honza

> +
> + /*
> + * Only clear out a write error when rewriting
> + */
> + if (test_set_buffer_req(bh) && (rw & WRITE))
> + clear_buffer_write_io_error(bh);
>
> bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
> bio->bi_bdev = bh->b_bdev;
--
Jan Kara <[email protected]>
SUSE Labs, CR