* [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
@ 2011-11-07 14:01 Namjae Jeon
2011-11-07 15:03 ` Srivatsa S. Bhat
2011-11-07 15:19 ` Ted Ts'o
0 siblings, 2 replies; 6+ messages in thread
From: Namjae Jeon @ 2011-11-07 14:01 UTC (permalink / raw)
To: tytso; +Cc: linux-kernel, linux-ext4, Namjae Jeon
Fix NULL pointer dereference from orig_data in fill_super and remount.
Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
---
fs/ext4/super.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9953d80..717b3e8 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3102,7 +3102,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
__releases(kernel_lock)
__acquires(kernel_lock)
{
- char *orig_data = kstrdup(data, GFP_KERNEL);
struct buffer_head *bh;
struct ext4_super_block *es = NULL;
struct ext4_sb_info *sbi;
@@ -3125,6 +3124,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
ext4_group_t first_not_zeroed;
+ char *orig_data = kstrdup(data, GFP_KERNEL);
+ if (!orig_data)
+ return ret;
+
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
goto out_free_orig;
@@ -4398,6 +4401,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
int i;
#endif
char *orig_data = kstrdup(data, GFP_KERNEL);
+ if (!orig_data)
+ return -ENOMEM;
/* Store the original options */
lock_super(sb);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-07 14:01 [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount Namjae Jeon
@ 2011-11-07 15:03 ` Srivatsa S. Bhat
2011-11-07 15:19 ` Ted Ts'o
1 sibling, 0 replies; 6+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-07 15:03 UTC (permalink / raw)
To: Namjae Jeon; +Cc: tytso, linux-kernel, linux-ext4
On 11/07/2011 07:31 PM, Namjae Jeon wrote:
> Fix NULL pointer dereference from orig_data in fill_super and remount.
>
> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> fs/ext4/super.c | 7 ++++++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9953d80..717b3e8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3102,7 +3102,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> __releases(kernel_lock)
> __acquires(kernel_lock)
> {
> - char *orig_data = kstrdup(data, GFP_KERNEL);
> struct buffer_head *bh;
> struct ext4_super_block *es = NULL;
> struct ext4_sb_info *sbi;
> @@ -3125,6 +3124,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> ext4_group_t first_not_zeroed;
>
> + char *orig_data = kstrdup(data, GFP_KERNEL);
> + if (!orig_data)
> + return ret;
> +
> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> if (!sbi)
> goto out_free_orig;
> @@ -4398,6 +4401,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> int i;
> #endif
> char *orig_data = kstrdup(data, GFP_KERNEL);
> + if (!orig_data)
> + return -ENOMEM;
>
> /* Store the original options */
> lock_super(sb);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-07 14:01 [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount Namjae Jeon
2011-11-07 15:03 ` Srivatsa S. Bhat
@ 2011-11-07 15:19 ` Ted Ts'o
2011-11-07 23:06 ` NamJae Jeon
1 sibling, 1 reply; 6+ messages in thread
From: Ted Ts'o @ 2011-11-07 15:19 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-kernel, linux-ext4
On Mon, Nov 07, 2011 at 11:01:51PM +0900, Namjae Jeon wrote:
> Fix NULL pointer dereference from orig_data in fill_super and remount.
>
> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
Um, did you even try to test this patch before submitting it?
#1: kstrdup() return NULL if data is NULL
#2: The kernel's printk and sprintf functions will translate
sprintf(buf, "%s", NULL) by filling in the string "NULL"
#3: The only use of orig_data is a cosmetic one, of printing out
the mount options which are passed in.
#4: If the user doesn't specify any mount options, data is NULL, and
your patch would cause those mounts to fail with ENOMEM.
In summary, there is no problem here; a pendant might complain that
it's possible for data to be non-NULL, and for orig_data to be null of
the kstrdup failed, but in that case, other memory allocations done
later in ext4_fill_super() will almost have certainly failed, so in
practice won't get as far as the printk.
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-07 15:19 ` Ted Ts'o
@ 2011-11-07 23:06 ` NamJae Jeon
2011-11-07 23:33 ` Theodore Tso
0 siblings, 1 reply; 6+ messages in thread
From: NamJae Jeon @ 2011-11-07 23:06 UTC (permalink / raw)
To: Ted Ts'o; +Cc: Namjae Jeon, linux-kernel, linux-ext4
2011/11/8 Ted Ts'o <tytso@mit.edu>:
> On Mon, Nov 07, 2011 at 11:01:51PM +0900, Namjae Jeon wrote:
>> Fix NULL pointer dereference from orig_data in fill_super and remount.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
>
> Um, did you even try to test this patch before submitting it?
>
> #1: kstrdup() return NULL if data is NULL
>
> #2: The kernel's printk and sprintf functions will translate
> sprintf(buf, "%s", NULL) by filling in the string "NULL"
>
> #3: The only use of orig_data is a cosmetic one, of printing out
> the mount options which are passed in.
>
> #4: If the user doesn't specify any mount options, data is NULL, and
> your patch would cause those mounts to fail with ENOMEM.
>
> In summary, there is no problem here; a pendant might complain that
> it's possible for data to be non-NULL, and for orig_data to be null of
> the kstrdup failed, but in that case, other memory allocations done
> later in ext4_fill_super() will almost have certainly failed, so in
> practice won't get as far as the printk.
Hi. Ted.
Sorry, I will make a patch next time after testing. Thanks for your review.
And I checked how to handle this case in other filesystem.
btrfs try to handle like this.
------------------------------------------------------------------------------
if (!options)
goto out;
/*
* strsep changes the string, duplicate it because parse_options
* gets called twice
*/
opts = kstrdup(options, GFP_KERNEL);
if (!opts)
return -ENOMEM;
-------------------------------------------------------------------------------
So I suggest that ext4 try to handle like this.
-------------------------------------------------------------------------------
if(data) {
orig_data = kstrdup(data, GFP_KERNEL);
if(!orig_data)
return ret;
}
-------------------------------------------------------------------------------
I want to know your opinion.
Thanks.
>
> - Ted
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-07 23:06 ` NamJae Jeon
@ 2011-11-07 23:33 ` Theodore Tso
2011-11-07 23:42 ` NamJae Jeon
0 siblings, 1 reply; 6+ messages in thread
From: Theodore Tso @ 2011-11-07 23:33 UTC (permalink / raw)
To: NamJae Jeon; +Cc: Theodore Tso, linux-kernel, linux-ext4
On Nov 7, 2011, at 6:06 PM, NamJae Jeon wrote:
> if(data) {
> orig_data = kstrdup(data, GFP_KERNEL);
> if(!orig_data)
> return ret;
> }
As I said earlier, the use of kstrdup() in the ext4 mount code is so that we can print the mount options used in the mount request. So if the kstrdup() fails, the worst that will happen is that the printk will print the the mount had no mount options, instead of the mount options which had been passed this might be misleading, but it's not the end of the world; it's only a cosmetic issue. But if the kstrdup() failed, the other memory allocations (that in general will be for much more memory) will also fail, so the mount will return ENOMEM before ever getting to the printjk. So a pendant might make the above change, but in practice it's not really that important to do that particular checking in this case.
This is why it's important to really understand the code in question instead of just posting patches without understanding the underlying code, and understanding what might or might not go wrong both before applying the patch, and after applying the patch. And of course, testing all of the use cases is also really important.
Best regards,
-- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-07 23:33 ` Theodore Tso
@ 2011-11-07 23:42 ` NamJae Jeon
0 siblings, 0 replies; 6+ messages in thread
From: NamJae Jeon @ 2011-11-07 23:42 UTC (permalink / raw)
To: Theodore Tso; +Cc: linux-kernel, linux-ext4
2011/11/8 Theodore Tso <tytso@mit.edu>:
>
> On Nov 7, 2011, at 6:06 PM, NamJae Jeon wrote:
>
>> if(data) {
>> orig_data = kstrdup(data, GFP_KERNEL);
>> if(!orig_data)
>> return ret;
>> }
>
> As I said earlier, the use of kstrdup() in the ext4 mount code is so that we can print the mount options used in the mount request. So if the kstrdup() fails, the worst that will happen is that the printk will print the the mount had no mount options, instead of the mount options which had been passed this might be misleading, but it's not the end of the world; it's only a cosmetic issue. But if the kstrdup() failed, the other memory allocations (that in general will be for much more memory) will also fail, so the mount will return ENOMEM before ever getting to the printjk. So a pendant might make the above change, but in practice it's not really that important to do that particular checking in this case.
>
> This is why it's important to really understand the code in question instead of just posting patches without understanding the underlying code, and understanding what might or might not go wrong both before applying the patch, and after applying the patch. And of course, testing all of the use cases is also really important.
Okay, I understand clearly.
Thanks.
>
> Best regards,
>
> -- Ted
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-07 23:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-07 14:01 [PATCH v2] ext4: fix NULL pointer dereference from orig_data in fill_super and remount Namjae Jeon
2011-11-07 15:03 ` Srivatsa S. Bhat
2011-11-07 15:19 ` Ted Ts'o
2011-11-07 23:06 ` NamJae Jeon
2011-11-07 23:33 ` Theodore Tso
2011-11-07 23:42 ` NamJae Jeon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).