linux-kernel - Re: [PATCH] fs: pstore: fix double-free on ramoops_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 07 Jan 2020 22:40:58 +0300
From:   Cengiz Can <cengiz@...nel.wtf>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH] fs: pstore: fix double-free on ramoops_init_przs

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.

-- 
Cengiz Can
@cengiz_io