| Message ID | 20220128202858.96935-1-vbendel@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | selinux: Fix and clean policydb->cond_list error paths | expand |
On Fri, Jan 28, 2022 at 9:29 PM <vbendel@redhat.com> 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@redhat.com> For the series (with or without the last patch): Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
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>