lkml.org 
[lkml]   [2007]   [May]   [4]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    DateFri, 04 May 2007 19:22:01 -0400
    FromFlorin Malita <>
    SubjectRe: [PATCH] UBI: dereference after kfree in create_vtbl
    Hi Satyam,
    
    Satyam Sharma wrote:
    > Eeks ... no, wait. You found a (two, actually) bug alright, but fixed
    > it wrong. When we fail a write, we *must* add it to the corrupted list
    > and _then_ attempt to retry. So, the "if (++tries <= 5)" applies to
    > "if (!err) goto retry;" and not to the ubi_scan_add_to_list(). The
    > difference is quite subtle here ...
    
    Not being familiar with the code, I was specifically trying to preserve 
    the old semantics and only address the use-after-free issue. So if there 
    was another bug... well, I guess I succeeded at preserving it ;)
    
    > The correct fix should actually be as follows: (Artem, this is diffed
    > on the original vtbl.c)
    [snip]
    > +    err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec, 
    > &si->corr);
    > +    kfree(new_seb);
    > +    if (++tries <= 5)
    >         if (!err)
    >             goto retry;
    
    There's a side effect to this change: by unconditionally overwriting err 
    we lose the original error code. Then if we're exceeding the number of 
    retries we'll end up returning 0 which is probably not what you want.
    
    Return code aside, it seems the only thing ubi_scan_add_to_list() is 
    doing is allocate a new struct ubi_scan_leb, initialize some fields with 
    values passed from new_seb and then add it to the desired list. But 
    copying new_seb to a newly allocated structure and then immediately 
    freeing the old one seems redundant - why not just add new_seb to the 
    corrupted list and be done? Then we don't have to deal with allocation 
    failures in an error path anymore - something like this (diff against 
    the original code):
    
    Signed-off-by: Florin Malita <fmalita@gmail.com>
    ---
    diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
    index b6fd6bb..2ad2d59 100644
    --- a/drivers/mtd/ubi/vtbl.c
    +++ b/drivers/mtd/ubi/vtbl.c
    @@ -317,14 +317,10 @@ retry:
     	return err;
     
     write_error:
    -	kfree(new_seb);
    -	/* May be this physical eraseblock went bad, try to pick another one */
    -	if (++tries <= 5) {
    -		err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
    -					   &si->corr);
    -		if (!err)
    -			goto retry;
    -	}
    +	/* Maybe this physical eraseblock went bad, try to pick another one */
    +	list_add_tail(&new_seb->u.list, &si->corr);
    +	if (++tries <= 5)
    +		goto retry;
     out_free:
     	ubi_free_vid_hdr(ubi, vid_hdr);
     	return err;
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at  http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at  http://www.tux.org/lkml/
    
    
    \
     
     \ /
      Last update: 2007-05-05 01:27    [from the cache]
    ©2003-2008