Messages in this thread | | | Date | Wed, 8 Jan 2020 10:01:04 -0800 | From | Kees Cook <> | Subject | Re: [PATCH] fs: pstore: fix double-free on ramoops_init_przs |
| |
On Tue, Jan 07, 2020 at 10:40:58PM +0300, Cengiz Can wrote: > Hello Kees! > > It's a pleasure to hear from you! > > On 2020-01-07 21:05, Kees Cook wrote: > > > > I think this is a false positive (have you actually hit the > > double-free?). The logic in this area is: > > No I did not actually hit the double-free. I'm just following > the indicators from static analyzer. > > > nothing was freeing the label on the failed prz, but all the other prz > > labels were free (i.e. there is a "i--" that skips the failed prz > > alloc). > > I have noticed that. Thanks for clearing it up though. > > The `kfree` I was referring to is in `err:` label of function > `persistent_ram_new` in `ram_core.c#595` of `for-next/pstore` tree: > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/tree/fs/pstore/ram_core.c?h=for-next/pstore#n595 > > Here are the relevant bits: > > ``` > struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t > size, > u32 sig, struct persistent_ram_ecc_info *ecc_info, > unsigned int memtype, u32 flags, char *label) > { > /* ... */ > /* ... */ > /* ... */ > return prz; > err: > persistent_ram_free(prz); /* <----- */ > return ERR_PTR(ret); > } > ``` > > So, to my understanding, if our `persistent_ram_new` call in `ram.c#583` > fails, it already does clean up the `label` pointer in itself and returns > an ERR_PTR back to us and our skipping logic does its job. > > I might be missing something but it seems so. > > Thank you for looking into this.
Ah-ha! Yes, I see it now. We have multiple paths to the err: label, and I was focused on the kzalloc() failure path. I will get this fixed better. Thanks!
-- Kees Cook
| |