2011/11/8 Ted Ts'o <tytso@xxxxxxx>: > 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@xxxxxxxxx> > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html