linux-kernel - Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171201172007.q2rqmo4jqaxb63tk@linux-n805>
Date:   Fri, 1 Dec 2017 09:20:07 -0800
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Philippe Mikoyan <philippe.mikoyan@...t.systems>
Cc:     akpm@...ux-foundation.org, viro@...iv.linux.org.uk,
        manfred@...orfullife.com, linux-kernel@...r.kernel.org,
        edgar.kaziakhmedov@...tuozzo.com
Subject: Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

On Thu, 30 Nov 2017, Philippe Mikoyan wrote:

>As described in the title, this patch fixes <ipc>id_ds inconsistency
>when <ipc>ctl_stat runs concurrently with some ds-changing function,
>e.g. shmat, msgsnd or whatever.
>
>For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
>following data structure can be returned:
>{... shm_lpid = 0, shm_nattch = 1, ...}

Hmm yeah that's pretty fishy, also shm_atime = 0, no?

So I think this patch is fine as we can obviously race at a user level.
This is another justification for converting the ipc lock to rwlock;
performance wise they are the pretty much the same (being queued)...
but that's irrelevant to this patch. I like that you manage to do
security and such checks still only under rcu, like all ipc calls
work; *_stat() is no longer special.

With a nit below:

Reviewed-by: Davidlohr Bueso <dbueso@...e.de>

>diff --git a/ipc/util.c b/ipc/util.c
>index 78755873cc5b..8d3c3946c825 100644
>--- a/ipc/util.c
>+++ b/ipc/util.c
>@@ -22,9 +22,12 @@
>  *	    tree.
>  *	    - perform initial checks (capabilities, auditing and permission,
>  *	      etc).
>- *	    - perform read-only operations, such as STAT, INFO commands.
>+ *	    - perform read-only operations, such as INFO command, that
>+ *	      do not demand atomicity
>  *	      acquire the ipc lock (kern_ipc_perm.lock) through
>  *	      ipc_lock_object()
>+ *		- perform read-only operatoins that demand atomicity,
                                          ^^ typo
>+ *		  such as STAT command.
>  *		- perform data updates, such as SET, RMID commands and
>  *		  mechanism-specific operations (semop/semtimedop,
>  *		  msgsnd/msgrcv, shmat/shmdt).

Thanks,
Davidlohr