From patchwork Fri Sep 8 22:20:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stephan Mueller X-Patchwork-Id: 9945113 X-Patchwork-Delegate: herbert@gondor.apana.org.au Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E0AC4604D4 for ; Fri, 8 Sep 2017 22:20:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D716328960 for ; Fri, 8 Sep 2017 22:20:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D57C32896B; Fri, 8 Sep 2017 22:20:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C37582897C for ; Fri, 8 Sep 2017 22:20:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757166AbdIHWUx convert rfc822-to-8bit (ORCPT ); Fri, 8 Sep 2017 18:20:53 -0400 Received: from mail.eperm.de ([89.247.134.16]:34672 "EHLO mail.eperm.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757154AbdIHWUx (ORCPT ); Fri, 8 Sep 2017 18:20:53 -0400 Received: from positron.chronox.de (unknown [46.234.225.113]) by mail.eperm.de (Postfix) with ESMTPA id F0A5C1816074; Sat, 9 Sep 2017 00:20:50 +0200 (CEST) From: Stephan =?ISO-8859-1?Q?M=FCller?= To: Herbert Xu , linux-crypto@vger.kernel.org Subject: [PATCH v2] crypto: add NULL check to scatterwalk_start Date: Sat, 09 Sep 2017 00:20:50 +0200 Message-ID: <6172557.07oG34Eo4q@positron.chronox.de> In-Reply-To: <20170907060108.GA21208@gondor.apana.org.au> References: <4634467.01PS30tDXi@positron.chronox.de> <3142753.SSoe9WBuHN@positron.chronox.de> <20170907060108.GA21208@gondor.apana.org.au> MIME-Version: 1.0 Sender: linux-crypto-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-crypto@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Am Donnerstag, 7. September 2017, 08:01:08 CEST schrieb Herbert Xu: Hi Herbert, > On Thu, Sep 07, 2017 at 07:48:53AM +0200, Stephan Müller wrote: > > There is already such check: > > > > static inline int crypto_aead_decrypt(struct aead_request *req) > > { > > > > struct crypto_aead *aead = crypto_aead_reqtfm(req); > > > > if (req->cryptlen < crypto_aead_authsize(aead)) > > > > return -EINVAL; > > > > ... > > That doesn't check assoclen, does it? > > > > Perhaps we can simply > > > truncate assoclen in aead_request_set_ad. > > > > I am not sure that would work because at the time we set the AAD len, we > > may not yet have cryptlen. I.e. aead_request_set_ad may be called before > > aead_request_set_crypt. > > We can add the truncation in both places. The initially suggested fix was wrong in general: cryptlen only defines the length of the ciphertext/plaintext without the AAD. This means, cryptlen can surely be less than AAD. The culprit is that in case authenc() is invoked from user space with AAD but with a zero plaintext. This in turn caused authenc() to invoke the CBC implementation with that zero plaintext which in turn simply starts a scatterwalk operation on the plaintext. This is in general appropriate as all code will handle zero lengths, except scatterwalk_start. This function assumes that there is always a valid SGL which is not true for for zero length input data. Granted, we could fix authenc to simply not invoke the CBC operation in the outlined issue. However, I now stumbled over this function for a third time in a row over the last weeks in bugs triggerable via AF_ALG. I suspect that there are many more issues like this lingering in other cipher implementation. Hence, I feel it is prudent to fix an entire class of bugs with this patch. Ciao Stephan ---8<--- In edge conditions, scatterwalk_start is invoked with an empty SGL. Although this is considered a wrong usage of scatterwalk_start, it can still be invoked. Such invocation can occur if the data to be covered by the SGL is zero. For example, if the authenc() cipher is invoked with an empty plaintext, the CBC operation is invoked with an empty plaintext. This patch fixes (at least) a crash that can be induced via AF_ALG from unprivileged user space. It can be argued whether authenc() should be changed to catch this issue. Yet, this issue in scatterwalk_start was a culprit in other kernel crash issues that have been fixed before invoking scatterwalk_start. As this function is constantly being invoked via AF_ALG from user space, harden the function to avoid a NULL pointer deference is prudent and even a general fix for these common issues. This fix therefore covers an entire class of bugs which are hard to chase down in their own individual cipher implementations. Fixes: ac02725812cb3 ("crypto: scatterwalk - Inline start/map/done") CC: CC: Herbert Xu Signed-off-by: Stephan Mueller --- include/crypto/scatterwalk.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h index 880e6be9e95e..0605d44b53bc 100644 --- a/include/crypto/scatterwalk.h +++ b/include/crypto/scatterwalk.h @@ -83,7 +83,10 @@ static inline void scatterwalk_start(struct scatter_walk *walk, struct scatterlist *sg) { walk->sg = sg; - walk->offset = sg->offset; + if (sg) + walk->offset = sg->offset; + else + walk->offset = 0; } static inline void *scatterwalk_map(struct scatter_walk *walk)