On Fri, Aug 21, 2020 at 11:47 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> In security_read_policy(), the policy length is computed using
> security_policydb_len(), which does a separate transaction, and then
> another transaction is done to write the policydb into a buffer of this
> length.
>
> The bug is that the policy might be re-loaded in between the two
> transactions and so the length can be wrong. In case the new length is
> lower than the old length, the length is corrected at the end of the
> function. In case the new length is higher than the old one, an error is
> returned.
>
> Fix it by doing everything in a single transaction and getting the
> length directly from policydb instead of calling
> security_policydb_len().
>
> Fixes: cee74f47a6ba ("SELinux: allow userspace to read policy back out of the kernel")
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> security/selinux/ss/services.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index a48fc1b337ba9..ab4de2a01634a 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3842,22 +3842,25 @@ int security_read_policy(struct selinux_state *state,
> void **data, size_t *len)
> {
> int rc;
> + struct policydb *policydb;
> struct policy_file fp;
>
> if (!selinux_initialized(state))
> return -EINVAL;
>
> - *len = security_policydb_len(state);
> + read_lock(&state->ss->policy_rwlock);
> + policydb = &state->ss->policy->policydb;
>
> + *len = policydb->len;
> *data = vmalloc_user(*len);
I don't believe you can hold a read_lock() across a vmalloc. That's
why this is done the way it is now.
> - if (!*data)
> - return -ENOMEM;
> -
> - fp.data = *data;
> - fp.len = *len;
> + if (!*data) {
> + rc = -ENOMEM;
> + } else {
> + fp.data = *data;
> + fp.len = *len;
>
> - read_lock(&state->ss->policy_rwlock);
> - rc = policydb_write(&state->ss->policy->policydb, &fp);
> + rc = policydb_write(policydb, &fp);
> + }
> read_unlock(&state->ss->policy_rwlock);
>
> if (rc)
> --
> 2.26.2
>
