diff mbox series

[SRU,P-ESM/T/Z,CVE-2017-11176] mqueue: fix a use-after-free in sys_mq_notify()

Message ID 20171005104748.29518-1-juerg.haefliger@canonical.com
State New
Headers show
Series [SRU,P-ESM/T/Z,CVE-2017-11176] mqueue: fix a use-after-free in sys_mq_notify() | expand

Commit Message

Juerg Haefliger Oct. 5, 2017, 10:47 a.m. UTC
From: Cong Wang <xiyou.wangcong@gmail.com>

The retry logic for netlink_attachskb() inside sys_mq_notify()
is nasty and vulnerable:

1) The sock refcnt is already released when retry is needed
2) The fd is controllable by user-space because we already
   release the file refcnt

so we when retry but the fd has been just closed by user-space
during this small window, we end up calling netlink_detachskb()
on the error path which releases the sock again, later when
the user-space closes this socket a use-after-free could be
triggered.

Setting 'sock' to NULL here should be sufficient to fix it.

CVE-2017-11176

Reported-by: GeneBlue <geneblue.mail@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: stable@kernel.org
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(cherry picked from commit f991af3daabaecff34684fd51fac80319d1baad1)
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
---
 ipc/mqueue.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Po-Hsu Lin Oct. 5, 2017, 11:42 a.m. UTC | #1
As this is just a medium CVE, I think Precise-ESM does not need this.

The fix looks reasonable, and clean cherry-pick to T/Z, thus:
Acked-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
Marcelo Henrique Cerri Oct. 5, 2017, 12:10 p.m. UTC | #2
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Kleber Sacilotto de Souza Oct. 5, 2017, 12:23 p.m. UTC | #3
On 10/05/2017 12:47 PM, Juerg Haefliger wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
>
> The retry logic for netlink_attachskb() inside sys_mq_notify()
> is nasty and vulnerable:
>
> 1) The sock refcnt is already released when retry is needed
> 2) The fd is controllable by user-space because we already
>    release the file refcnt
>
> so we when retry but the fd has been just closed by user-space
> during this small window, we end up calling netlink_detachskb()
> on the error path which releases the sock again, later when
> the user-space closes this socket a use-after-free could be
> triggered.
>
> Setting 'sock' to NULL here should be sufficient to fix it.
>
> CVE-2017-11176
>
> Reported-by: GeneBlue <geneblue.mail@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Manfred Spraul <manfred@colorfullife.com>
> Cc: stable@kernel.org
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry picked from commit f991af3daabaecff34684fd51fac80319d1baad1)
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>

As pointed out by Po-Hsu Lin, this fix is not needed for 
precise-esm/master, only for precise-esm/lts-trusty. So for T and Z:

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>


> ---
>  ipc/mqueue.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 5b4293d9819d..081a2d74b0d1 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -1095,8 +1095,10 @@ retry:
>
>  			timeo = MAX_SCHEDULE_TIMEOUT;
>  			ret = netlink_attachskb(sock, nc, &timeo, NULL);
> -			if (ret == 1)
> +			if (ret == 1) {
> +				sock = NULL;
>  				goto retry;
> +			}
>  			if (ret) {
>  				sock = NULL;
>  				nc = NULL;
>
Thadeu Lima de Souza Cascardo Oct. 6, 2017, 6:44 p.m. UTC | #4
Applied to trusty and zesty master-next branches.

Thanks.
Cascardo.

Applied-to: trusty/master-next
Applied-to: zesty/master-next
diff mbox series

Patch

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 5b4293d9819d..081a2d74b0d1 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -1095,8 +1095,10 @@  retry:
 
 			timeo = MAX_SCHEDULE_TIMEOUT;
 			ret = netlink_attachskb(sock, nc, &timeo, NULL);
-			if (ret == 1)
+			if (ret == 1) {
+				sock = NULL;
 				goto retry;
+			}
 			if (ret) {
 				sock = NULL;
 				nc = NULL;