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@xxxxxxxxxx>
