* [PATCH] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
@ 2011-11-06 15:19 Namjae Jeon
2011-11-06 20:33 ` Srivatsa S. Bhat
0 siblings, 1 reply; 4+ messages in thread
From: Namjae Jeon @ 2011-11-06 15:19 UTC (permalink / raw)
To: tytso; +Cc: linux-kernel, 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 | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9953d80..3770d3f 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;
@@ -3124,6 +3123,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
int err;
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)
@@ -4398,6 +4401,10 @@ 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) {
+ err = -ENOMEM;
+ goto failed_alloc_orig;
+ }
/* Store the original options */
lock_super(sb);
@@ -4562,6 +4569,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
return 0;
restore_opts:
+ kfree(orig_data);
+failed_alloc_orig:
sb->s_flags = old_sb_flags;
sbi->s_mount_opt = old_opts.s_mount_opt;
sbi->s_mount_opt2 = old_opts.s_mount_opt2;
@@ -4580,7 +4589,6 @@ restore_opts:
}
#endif
unlock_super(sb);
- kfree(orig_data);
return err;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-06 15:19 [PATCH] ext4: fix NULL pointer dereference from orig_data in fill_super and remount Namjae Jeon
@ 2011-11-06 20:33 ` Srivatsa S. Bhat
2011-11-06 23:13 ` NamJae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-06 20:33 UTC (permalink / raw)
To: Namjae Jeon; +Cc: tytso, linux-kernel
On 11/06/2011 08:49 PM, Namjae Jeon wrote:
> Fix NULL pointer dereference from orig_data in fill_super and remount.
>
> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
> ---
> fs/ext4/super.c | 12 ++++++++++--
> 1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9953d80..3770d3f 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;
> @@ -3124,6 +3123,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> int err;
> 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)
> @@ -4398,6 +4401,10 @@ 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) {
> + err = -ENOMEM;
> + goto failed_alloc_orig;
I didn't quite get why 'failed_alloc_orig' is needed here.
Why can't we just return -ENOMEM? Anyway we haven't yet
clobbered any opts at this point, right?
See my comments below.
> + }
>
> /* Store the original options */
> lock_super(sb);
> @@ -4562,6 +4569,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> return 0;
>
> restore_opts:
> + kfree(orig_data);
> +failed_alloc_orig:
> sb->s_flags = old_sb_flags;
> sbi->s_mount_opt = old_opts.s_mount_opt;
> sbi->s_mount_opt2 = old_opts.s_mount_opt2;
This would put garbage values in sb->... and sbi->... when we jump
to 'failed_alloc_orig' upon 'orig_data' allocation failure, because
the old_* variables were still uninitialized at that point.
> @@ -4580,7 +4589,6 @@ restore_opts:
> }
> #endif
> unlock_super(sb);
> - kfree(orig_data);
> return err;
> }
>
Thanks,
Srivatsa S. Bhat
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-06 20:33 ` Srivatsa S. Bhat
@ 2011-11-06 23:13 ` NamJae Jeon
2011-11-06 23:20 ` Srivatsa S. Bhat
0 siblings, 1 reply; 4+ messages in thread
From: NamJae Jeon @ 2011-11-06 23:13 UTC (permalink / raw)
To: Srivatsa S. Bhat; +Cc: tytso, linux-kernel
2011/11/7 Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>:
> On 11/06/2011 08:49 PM, Namjae Jeon wrote:
>> Fix NULL pointer dereference from orig_data in fill_super and remount.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
>> ---
>> fs/ext4/super.c | 12 ++++++++++--
>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 9953d80..3770d3f 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;
>> @@ -3124,6 +3123,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> int err;
>> 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)
>> @@ -4398,6 +4401,10 @@ 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) {
>> + err = -ENOMEM;
>> + goto failed_alloc_orig;
>
> I didn't quite get why 'failed_alloc_orig' is needed here.
> Why can't we just return -ENOMEM? Anyway we haven't yet
> clobbered any opts at this point, right?
>
> See my comments below.
>
>> + }
>>
>> /* Store the original options */
>> lock_super(sb);
>> @@ -4562,6 +4569,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>> return 0;
>>
>> restore_opts:
>> + kfree(orig_data);
>> +failed_alloc_orig:
>> sb->s_flags = old_sb_flags;
>> sbi->s_mount_opt = old_opts.s_mount_opt;
>> sbi->s_mount_opt2 = old_opts.s_mount_opt2;
>
> This would put garbage values in sb->... and sbi->... when we jump
> to 'failed_alloc_orig' upon 'orig_data' allocation failure, because
> the old_* variables were still uninitialized at that point.
>
>> @@ -4580,7 +4589,6 @@ restore_opts:
>> }
>> #endif
>> unlock_super(sb);
>> - kfree(orig_data);
>> return err;
>> }
>>
>
> Thanks,
> Srivatsa S. Bhat
Hi.
Thanks for your review. It's my mistake.
I will post patch v2 after modifing again.
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: fix NULL pointer dereference from orig_data in fill_super and remount.
2011-11-06 23:13 ` NamJae Jeon
@ 2011-11-06 23:20 ` Srivatsa S. Bhat
0 siblings, 0 replies; 4+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-06 23:20 UTC (permalink / raw)
To: NamJae Jeon; +Cc: tytso, linux-kernel, linux-ext4
On 11/07/2011 04:43 AM, NamJae Jeon wrote:
> 2011/11/7 Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>:
>> On 11/06/2011 08:49 PM, Namjae Jeon wrote:
>>> Fix NULL pointer dereference from orig_data in fill_super and remount.
>>>
>>> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
>>> ---
>>> fs/ext4/super.c | 12 ++++++++++--
>>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index 9953d80..3770d3f 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;
>>> @@ -3124,6 +3123,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>>> int err;
>>> 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)
>>> @@ -4398,6 +4401,10 @@ 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) {
>>> + err = -ENOMEM;
>>> + goto failed_alloc_orig;
>>
>> I didn't quite get why 'failed_alloc_orig' is needed here.
>> Why can't we just return -ENOMEM? Anyway we haven't yet
>> clobbered any opts at this point, right?
>>
>> See my comments below.
>>
>>> + }
>>>
>>> /* Store the original options */
>>> lock_super(sb);
>>> @@ -4562,6 +4569,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>>> return 0;
>>>
>>> restore_opts:
>>> + kfree(orig_data);
>>> +failed_alloc_orig:
>>> sb->s_flags = old_sb_flags;
>>> sbi->s_mount_opt = old_opts.s_mount_opt;
>>> sbi->s_mount_opt2 = old_opts.s_mount_opt2;
>>
>> This would put garbage values in sb->... and sbi->... when we jump
>> to 'failed_alloc_orig' upon 'orig_data' allocation failure, because
>> the old_* variables were still uninitialized at that point.
>>
>>> @@ -4580,7 +4589,6 @@ restore_opts:
>>> }
>>> #endif
>>> unlock_super(sb);
>>> - kfree(orig_data);
>>> return err;
>>> }
>>>
>>
>> Thanks,
>> Srivatsa S. Bhat
> Hi.
> Thanks for your review. It's my mistake.
> I will post patch v2 after modifing again.
Sure. And please CC linux-ext4@vger.kernel.org for ext4 related patches.
Thanks,
Srivatsa S. Bhat
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-11-06 23:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-06 15:19 [PATCH] ext4: fix NULL pointer dereference from orig_data in fill_super and remount Namjae Jeon
2011-11-06 20:33 ` Srivatsa S. Bhat
2011-11-06 23:13 ` NamJae Jeon
2011-11-06 23:20 ` Srivatsa S. Bhat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox