From patchwork Thu Feb 16 15:02:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Seth Forshee X-Patchwork-Id: 728727 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 3vPKBq1Q19z9s8S; Fri, 17 Feb 2017 02:02:27 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical-com.20150623.gappssmtp.com header.i=@canonical-com.20150623.gappssmtp.com header.b="WY20AdDC"; dkim-atps=neutral Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1ceNZe-0000wS-HK; Thu, 16 Feb 2017 15:02:22 +0000 Received: from mail-oi0-f42.google.com ([209.85.218.42]) by huckleberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1ceNZZ-0000wI-9y for kernel-team@lists.ubuntu.com; Thu, 16 Feb 2017 15:02:17 +0000 Received: by mail-oi0-f42.google.com with SMTP id u143so9490342oif.3 for ; Thu, 16 Feb 2017 07:02:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical-com.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id; bh=9Hf8TlW2IJpD+wgf5sIjxizpkgdMhO+f7Z9Jeet+a4k=; b=WY20AdDCeMLq2LYPrevFpd5whSYbDxgbqk+WG+0lff/hpmqjXeimCEdO1DfjP+rfkw aYZIubk8xlxrR596spj2AffalY9GzdwSWAbl9sAj+8xV0MPV2kQl109WeEeKfXML40Uc E9I9SeekFhY3rBJWKT3vCj6iOltmfA46DrWTxBN8tpdfnTlSGRvyKHLzKMhA6o1N4Q4q w8EjZ5LgfZ16J4eFfS7L5qgdQFGSfbtiLhcFpFJWjPcoE43sq9yaAZCUSIiJEzSODTfl mp72T3HaQmDTAE9a7x8Sp80BeegVuotwKgTA+P99dkgvh+IzRAm17EzVb8vxQbuNE//I WPTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=9Hf8TlW2IJpD+wgf5sIjxizpkgdMhO+f7Z9Jeet+a4k=; b=NutGMH/7BtUzgkc0f5QXovdiJQPPIFAKs0NJ+FVx8vPPqBr0ryqkf11oYa4iMeuxc6 K53ElZteH0BkgqCuGJiIz3WWquFNahFs5JrEZqADbJEtE88X15Yj+4SLHM1+O1E31nOA 0ufcM9FGAlHFbhN1FanUQOaalMCUpjMSB0xWQcCqYO3PpR8rROQoG7rrAslQfrEt3FQt Oh8sHjP+yewAQV0cEBQ8xC48zxg48kNC/u6BKKlnpjhK8gQt/pzuKQb6LLmA0PlZ0S1R Xku83Y+Uxkr8LZ1Ba4YAPhLL8nYeYRONptYxPd215HUahRwx14/GZCIISkbRRJm0EKq9 7Mqg== X-Gm-Message-State: AMke39l3EaZu4/JrPNCW2NaIXexrGnjK61TUtSxPVWXRNSZsqHCLgBqmN1KGaEBzxOS+4KMM X-Received: by 10.107.195.143 with SMTP id t137mr2459974iof.46.1487257335758; Thu, 16 Feb 2017 07:02:15 -0800 (PST) Received: from localhost ([2605:a601:aa7:8220:d525:2d8c:d0b4:d509]) by smtp.gmail.com with ESMTPSA id s1sm4299721itb.21.2017.02.16.07.02.14 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 16 Feb 2017 07:02:15 -0800 (PST) From: Seth Forshee To: kernel-team@lists.ubuntu.com Subject: [PATCH][Xenial SRU] SUNRPC: fix refcounting problems with auth_gss messages. Date: Thu, 16 Feb 2017 09:02:14 -0600 Message-Id: <1487257334-71690-1-git-send-email-seth.forshee@canonical.com> X-Mailer: git-send-email 2.7.4 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com From: NeilBrown BugLink: http://bugs.launchpad.net/bugs/1650336 There are two problems with refcounting of auth_gss messages. First, the reference on the pipe->pipe list (taken by a call to rpc_queue_upcall()) is not counted. It seems to be assumed that a message in pipe->pipe will always also be in pipe->in_downcall, where it is correctly reference counted. However there is no guaranty of this. I have a report of a NULL dereferences in rpc_pipe_read() which suggests a msg that has been freed is still on the pipe->pipe list. One way I imagine this might happen is: - message is queued for uid=U and auth->service=S1 - rpc.gssd reads this message and starts processing. This removes the message from pipe->pipe - message is queued for uid=U and auth->service=S2 - rpc.gssd replies to the first message. gss_pipe_downcall() calls __gss_find_upcall(pipe, U, NULL) and it finds the *second* message, as new messages are placed at the head of ->in_downcall, and the service type is not checked. - This second message is removed from ->in_downcall and freed by gss_release_msg() (even though it is still on pipe->pipe) - rpc.gssd tries to read another message, and dereferences a pointer to this message that has just been freed. I fix this by incrementing the reference count before calling rpc_queue_upcall(), and decrementing it if that fails, or normally in gss_pipe_destroy_msg(). It seems strange that the reply doesn't target the message more precisely, but I don't know all the details. In any case, I think the reference counting irregularity became a measureable bug when the extra arg was added to __gss_find_upcall(), hence the Fixes: line below. The second problem is that if rpc_queue_upcall() fails, the new message is not freed. gss_alloc_msg() set the ->count to 1, gss_add_msg() increments this to 2, gss_unhash_msg() decrements to 1, then the pointer is discarded so the memory never gets freed. Fixes: 9130b8dbc6ac ("SUNRPC: allow for upcalls for same uid but different gss service") Cc: stable@vger.kernel.org Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1011250 Signed-off-by: NeilBrown Signed-off-by: Trond Myklebust (cherry picked from commit 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c) Signed-off-by: Seth Forshee --- net/sunrpc/auth_gss/auth_gss.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c index 06095cc8815e..1f0687d8e3d7 100644 --- a/net/sunrpc/auth_gss/auth_gss.c +++ b/net/sunrpc/auth_gss/auth_gss.c @@ -541,9 +541,13 @@ gss_setup_upcall(struct gss_auth *gss_auth, struct rpc_cred *cred) return gss_new; gss_msg = gss_add_msg(gss_new); if (gss_msg == gss_new) { - int res = rpc_queue_upcall(gss_new->pipe, &gss_new->msg); + int res; + atomic_inc(&gss_msg->count); + res = rpc_queue_upcall(gss_new->pipe, &gss_new->msg); if (res) { gss_unhash_msg(gss_new); + atomic_dec(&gss_msg->count); + gss_release_msg(gss_new); gss_msg = ERR_PTR(res); } } else @@ -836,6 +840,7 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg) warn_gssd(); gss_release_msg(gss_msg); } + gss_release_msg(gss_msg); } static void gss_pipe_dentry_destroy(struct dentry *dir,