On Fri, Jan 28, 2022 at 9:29 PM <vbendel@xxxxxxxxxx> wrote: > 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> For the series (with or without the last patch): Reviewed-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
