Messages in this thread | | | From | vbendel@redhat ... | Subject | [PATCH 0/3] selinux: Fix and clean policydb->cond_list error paths | Date | Fri, 28 Jan 2022 21:28:55 +0100 |
| |
There are two users of policydb->cond_list: cond_read_list() and duplicate_policydb_cond_list(). If any of them gets an error, usually an -ENOMEM, the error-path-cleanup *_destroy() functions get called twice: firstly from these two and secondly from the caller functions' error paths.
In case such -ENOMEM happens while assigning cond_node data, i.e. while ->cond_list_len is already non-zero, it leads to inappropriate dereferencing of policydb->cond_list[] data in the second called cond_list_destroy() from the caller functions' error paths, resulting with: - NULL pointer deref from cond_read_list(); - use-after-free + double-free from duplicate_policydb_cond_list(). (the cond_read_list() manages to set ->cond_list to NULL)
Patch 1/3 simply makes the error behavior consistent by always setting ->cond_list to NULL.
Patch 2/3 fixes the actual bug by resetting ->cond_list_len to 0, so any subsequent cond_list_destroy() calls would become noop.
Patch 3/3 cleans up the duplicate *_destroy calls on these error paths, albeit it's a bit questionable and I'm looking for feedback on it: - on one hand the idea is that the caller functions call the *_destroy() bits anyway, hence removing duplicate efforts (which also fixes the bug, but I'd still prefer to apply patches 1 and 2 regardless); - on the other hand it's appropriate and more bug-proof for a function to clean everything it allocated on error. Hence I'm looking forward to seeing what approach the upstream would find more appropriate.
Signed-off-by: Vratislav Bendel <vbendel@redhat.com>
| |