From patchwork Fri Jan 12 14:42:28 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Lamparter X-Patchwork-Id: 10161187 X-Patchwork-Delegate: kvalo@adurom.com 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 E33CF60327 for ; Fri, 12 Jan 2018 14:42:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D37FB287ED for ; Fri, 12 Jan 2018 14:42:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C6B2328830; Fri, 12 Jan 2018 14:42:53 +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=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_HI autolearn=unavailable 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 4037F287ED for ; Fri, 12 Jan 2018 14:42:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933975AbeALOme (ORCPT ); Fri, 12 Jan 2018 09:42:34 -0500 Received: from mail-wr0-f193.google.com ([209.85.128.193]:43139 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933757AbeALOmc (ORCPT ); Fri, 12 Jan 2018 09:42:32 -0500 Received: by mail-wr0-f193.google.com with SMTP id s13so5484167wra.10; Fri, 12 Jan 2018 06:42:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=x88s7hzEbU/wjKh0L+/4gjOCM4J6TocKd8xqcCm26jQ=; b=h3cZZFdd7P9xIFIbsmfIfDlOXbfPSKBfMQQJZBV+Tq4bqcP66KrTSqqyoPGj5j7nEt ASzlXskB6/eC+trier833gMn349AbZMJvE07KHb3HH37zsxA1S3fe+4HECWCtPruzwjj dyJR7sYFpQ6qOVDMxR+2nknJ0QT0An56bW5caFZBHWOCZCzNSa1jyVc/36TkeZGNVvKB RaorthRYQmeTDiVGm7eVIJ5puu1WI1kGn73nUYvOKNVkUVdHeLMueZdTjps0Stc3DqQx sdPzeP5U/m84m10OosEzs4O5elAS5aWsXasGLnAUihdj+AyK0SSHvgkFdSdCJ4atYNYV l+Uw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=x88s7hzEbU/wjKh0L+/4gjOCM4J6TocKd8xqcCm26jQ=; b=jmEESoubrdNJH2U4zn3WceDaTuBK6svsPCSbp1BXmJZWxK+9rweuJtV0VCb5jqDvXe uN0MtAhxvo115FSaKZ0mE5iJ++Pv1d+AL/M30FMFwGXZ6EP8fVTn9WC8FTyHt7A8os4F TEdjgq3voQSjqq/IiDcQXbHhSwoMP7asHLn2RBRAkby0JWJl5YZ/7LHM2djv6abLJgyd uFN9gCMB+T3pKeqF2J9HIZaBd2v2sIr/abdEZQUWDZaKrsKYGbKqaIeC3t7+h0ybOsjr ZvtKXelSkGU67kNa/FiQ8rsn+nCXLcArvw+GsbBczjL1Az5o9n0arKQRWXrkHF81fjee 7cYQ== X-Gm-Message-State: AKGB3mIo+mMy3CtVtGWeO/1HjTxJnvWPweyKyTOZZIlWRjbFRE7V3Q9s o6i4tEj+nXJHZ2k1zfx85fKjUW61J8Q= X-Google-Smtp-Source: ACJfBoueE/KRKFbC9+MsJBq4ODcAivqQwcHZefU6vD7mn71etAfLrTUZ898pEjOHFS3/l5RMSU0vWQ== X-Received: by 10.223.174.236 with SMTP id y99mr15586110wrc.117.1515768150501; Fri, 12 Jan 2018 06:42:30 -0800 (PST) Received: from debian64.daheim (p200300D5FBC477FC0000000000000830.dip0.t-ipconnect.de. [2003:d5:fbc4:77fc::830]) by smtp.gmail.com with ESMTPSA id k125sm6256590wmd.48.2018.01.12.06.42.29 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 12 Jan 2018 06:42:29 -0800 (PST) From: Christian Lamparter X-Google-Original-From: Christian Lamparter Received: from localhost.daheim ([127.0.0.1] helo=debian64.localnet) by debian64.daheim with esmtp (Exim 4.90) (envelope-from ) id 1ea0XM-0001kt-Ix; Fri, 12 Jan 2018 15:42:28 +0100 To: Dan Williams Cc: linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, kernel-hardening@lists.openwall.com, netdev@vger.kernel.org, linux-wireless@vger.kernel.org, Elena Reshetova , tglx@linutronix.de, torvalds@linux-foundation.org, akpm@linux-foundation.org, Kalle Valo , alan@linux.intel.com Subject: Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution Date: Fri, 12 Jan 2018 15:42:28 +0100 Message-ID: <1648304.tjl4HeBnOe@debian64> In-Reply-To: <151571806633.27429.1504260808341642890.stgit@dwillia2-desk3.amr.corp.intel.com> References: <151571798296.27429.7166552848688034184.stgit@dwillia2-desk3.amr.corp.intel.com> <151571806633.27429.1504260808341642890.stgit@dwillia2-desk3.amr.corp.intel.com> MIME-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote: > Static analysis reports that 'queue' may be a user controlled value that > is used as a data dependency to read from the 'ar9170_qmap' array. In > order to avoid potential leaks of kernel memory values, block > speculative execution of the instruction stream that could issue reads > based on an invalid result of 'ar9170_qmap[queue]'. In this case the > value of 'ar9170_qmap[queue]' is immediately reused as an index to the > 'ar->edcf' array. > > Based on an original patch by Elena Reshetova. > > Cc: Christian Lamparter > Cc: Kalle Valo > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Elena Reshetova > Signed-off-by: Dan Williams > --- This patch (and p54, cw1200) look like the same patch?! Can you tell me what happend to: On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote: > On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter wrote: > > And Furthermore a invalid queue (param->ac) would cause a crash in > > this line in mac80211 before it even reaches the driver [1]: > > | sdata->tx_conf[params->ac] = p; > > | ^^^^^^^^ > > | if (drv_conf_tx(local, sdata, >>>> params->ac <<<<, &p)) { > > | ^^ (this is a wrapper for the *_op_conf_tx) > > > > I don't think these chin-up exercises are needed. > > Quite the contrary, you've identified a better place in the call stack > to sanitize the input and disable speculation. Then we can kill the > whole class of the wireless driver reports at once it seems. Anyway, I think there's an easy way to solve this: remove the "if (queue < ar->hw->queues)" check altogether. It's no longer needed anymore as the "queue" value is validated long before the driver code gets called. And from my understanding, this will fix the "In this case the value of 'ar9170_qmap[queue]' is immediately reused as an index to the 'ar->edcf' array." gripe your tool complains about. This is here's a quick test-case for carl9170.: --- --- What does your tool say about this? (If necessary, the "queue" value could be even sanitized with a queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.) diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index 988c8857d78c..2d3afb15bb62 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, int ret; mutex_lock(&ar->mutex); - if (queue < ar->hw->queues) { - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); - ret = carl9170_set_qos(ar); - } else { - ret = -EINVAL; - } - + memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); + ret = carl9170_set_qos(ar); mutex_unlock(&ar->mutex); return ret; }