On Wed, Jun 21, 2023 at 1:12 PM Pablo Neira Ayuso pablo@netfilter.org wrote:
On Wed, Jun 21, 2023 at 12:20:44PM +0200, Florent Revest wrote:
On Tue, Jun 20, 2023 at 8:35 AM Pablo Neira Ayuso pablo@netfilter.org wrote:
nf_conntrack_ftp depends on nf_conntrack.
If nf_conntrack fails to load, how can nf_conntrack_ftp be loaded?
Is this maybe only true of dynamically loaded kmods ? With CONFIG_NF_CONNTRACK_FTP=y, it seems to me that nf_conntrack_ftp_init() will be called as an __init function, independently of whether nf_conntrack_init_start() succeeded or not. Am I missing something ?
No idea, nf_conntrack init path invokes nf_conntrack_helper_init() which initializes the helper hashtable.
Yes
How is it that you can nf_conntrack_helpers_register() call before the initialization path of nf_conntrack is run, that I don't know.
No, this is not what happens. I tried to describe the problem in the following paragraph (I'm open to suggestions on how to explain this better!)
On Thu, Jun 15, 2023 at 05:29:18PM +0200, Florent Revest wrote:
If register_nf_conntrack_bpf() fails (for example, if the .BTF section contains an invalid entry), nf_conntrack_init_start() calls nf_conntrack_helper_fini() as part of its cleanup path and nf_ct_helper_hash gets freed.
Said differently (chronologically, I hope that helps)
First, nf_conntrack_init_start() runs: - it calls nf_conntrack_helper_init() (this succeeds and initializes the hashmap) - it calls register_nf_conntrack_bpf() (this fails) - goto err_kfunc - it calls nf_conntrack_helper_fini() (this frees the hashmap and leaves a dangling nf_ct_helper_hash pointer) - it returns back an error such that nf_conntrack_standalone_init() fails
At this point, the builtin nf_conntrack module failed to load.
But now, nf_conntrack_ftp_init() also runs: - it calls nf_conntrack_helpers_register() - this calls nf_conntrack_helper_register() - this accesses the hashmap pointer even though the hashmap has been freed already. That's where the use-after-free is.
I proposed we fix this by not accessing a freed hashmap (using NULL as a clear indication that this is now an invalid pointer) but I suppose there are other ways one could go about it such as checking if nf_conntrack initialized successfully early in nf_conntrack_ftp_init() etc... I'm open to suggestions.