| Message ID | 1320674511-1980-1-git-send-email-linkinjeon@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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); -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)