[PATCH] iwlwifi: actually check allocated conf_tlv pointer

[PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Chris Rorvick]

Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
("iwlwifi: dbg: move debug data to a struct") but does not implement the
check correctly.

Tweeted-by: @grsecurity
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
In this wasn't picked up?

 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index ff52e69c1c80..a37f330e7bd4 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1465,11 +1465,11 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 		if (pieces->dbg_conf_tlv[i]) {
 			drv->fw.dbg.conf_tlv[i] =
 				kmemdup(pieces->dbg_conf_tlv[i],
 					pieces->dbg_conf_tlv_len[i],
 					GFP_KERNEL);
-			if (!pieces->dbg_conf_tlv[i])
+			if (!drv->fw.dbg_conf_tlv[i])
 				goto out_free_fw;
 		}
 	}
 
 	memset(&trigger_tlv_sz, 0xff, sizeof(trigger_tlv_sz));
-- 
2.24.1

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Kalle Valo]

Chris Rorvick <chris@rorvick.com> writes:

> Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
> conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
> ("iwlwifi: dbg: move debug data to a struct") but does not implement the
> check correctly.
>
> Tweeted-by: @grsecurity
> Signed-off-by: Chris Rorvick <chris@rorvick.com>

I'll add:

Fixes: 71bc0334a637 ("iwlwifi: check allocated pointer when allocating conf_tlvs")

> ---
> In this wasn't picked up?

Luca, can I take this directly?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Luca Coelho]

On Sun, 2020-04-05 at 11:44 +0300, Kalle Valo wrote:
> Chris Rorvick <chris@rorvick.com> writes:
> 
> > Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
> > conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
> > ("iwlwifi: dbg: move debug data to a struct") but does not implement the
> > check correctly.
> > 
> > Tweeted-by: @grsecurity
> > Signed-off-by: Chris Rorvick <chris@rorvick.com>
> 
> I'll add:
> 
> Fixes: 71bc0334a637 ("iwlwifi: check allocated pointer when allocating conf_tlvs")
> 
> > ---
> > In this wasn't picked up?
> 
> Luca, can I take this directly?

Yes, please take it directly.  This can happen in OOM situations and,
when it does, we will potentially try to dereference a NULL pointer.

Thanks!

--
Cheers,
Luca.

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Kalle Valo]

Luca Coelho <luca@coelho.fi> writes:

> On Sun, 2020-04-05 at 11:44 +0300, Kalle Valo wrote:
>> Chris Rorvick <chris@rorvick.com> writes:
>> 
>> > Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
>> > conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
>> > ("iwlwifi: dbg: move debug data to a struct") but does not implement the
>> > check correctly.
>> > 
>> > Tweeted-by: @grsecurity
>> > Signed-off-by: Chris Rorvick <chris@rorvick.com>
>> 
>> I'll add:
>> 
>> Fixes: 71bc0334a637 ("iwlwifi: check allocated pointer when allocating conf_tlvs")
>> 
>> > ---
>> > In this wasn't picked up?
>> 
>> Luca, can I take this directly?
>
> Yes, please take it directly.

Ok, assigned it to me in patchwork.

> This can happen in OOM situations and, when it does, we will
> potentially try to dereference a NULL pointer.

I'll add this to the commit log.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Kalle Valo]

Chris Rorvick <chris@rorvick.com> wrote:

> Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
> conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
> ("iwlwifi: dbg: move debug data to a struct") but does not implement the
> check correctly.
> 
> This can happen in OOM situations and, when it does, we will potentially try to
> dereference a NULL pointer.
> 
> Tweeted-by: @grsecurity
> Signed-off-by: Chris Rorvick <chris@rorvick.com>

Fails to build, please rebase on top of wireless-drivers.

drivers/net/wireless/intel/iwlwifi/iwl-drv.c: In function 'iwl_req_fw_callback':
drivers/net/wireless/intel/iwlwifi/iwl-drv.c:1470:16: error: 'struct iwl_fw' has no member named 'dbg_conf_tlv'
    if (!drv->fw.dbg_conf_tlv[i])
                ^
make[5]: *** [drivers/net/wireless/intel/iwlwifi/iwl-drv.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [drivers/net/wireless/intel/iwlwifi] Error 2
make[3]: *** [drivers/net/wireless/intel] Error 2
make[2]: *** [drivers/net/wireless] Error 2
make[1]: *** [drivers/net] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/patch/11470125/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Sedat Dilek]

On Mon, Apr 6, 2020 at 4:11 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Chris Rorvick <chris@rorvick.com> wrote:
>
> > Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
> > conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
> > ("iwlwifi: dbg: move debug data to a struct") but does not implement the
> > check correctly.
> >
> > This can happen in OOM situations and, when it does, we will potentially try to
> > dereference a NULL pointer.
> >
> > Tweeted-by: @grsecurity
> > Signed-off-by: Chris Rorvick <chris@rorvick.com>
>
> Fails to build, please rebase on top of wireless-drivers.
>
> drivers/net/wireless/intel/iwlwifi/iwl-drv.c: In function 'iwl_req_fw_callback':
> drivers/net/wireless/intel/iwlwifi/iwl-drv.c:1470:16: error: 'struct iwl_fw' has no member named 'dbg_conf_tlv'
>     if (!drv->fw.dbg_conf_tlv[i])
>                 ^
> make[5]: *** [drivers/net/wireless/intel/iwlwifi/iwl-drv.o] Error 1
> make[5]: *** Waiting for unfinished jobs....
> make[4]: *** [drivers/net/wireless/intel/iwlwifi] Error 2
> make[3]: *** [drivers/net/wireless/intel] Error 2
> make[2]: *** [drivers/net/wireless] Error 2
> make[1]: *** [drivers/net] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [drivers] Error 2
>

Should be:

$ git diff
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 0481796f75bc..c24350222133 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1467,7 +1467,7 @@ static void iwl_req_fw_callback(const struct
firmware *ucode_raw, void *context)
                                kmemdup(pieces->dbg_conf_tlv[i],
                                        pieces->dbg_conf_tlv_len[i],
                                        GFP_KERNEL);
-                       if (!pieces->dbg_conf_tlv[i])
+                       if (!drv->fw.dbg.conf_tlv[i])
                                goto out_free_fw;
                }
        }

"fw.dbg.conf" with a dot not underscore.

- Sedat -



> Patch set to Changes Requested.
>
> --
> https://patchwork.kernel.org/patch/11470125/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Sedat Dilek]

On Mon, Apr 6, 2020 at 9:53 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Mon, Apr 6, 2020 at 4:11 PM Kalle Valo <kvalo@codeaurora.org> wrote:
> >
> > Chris Rorvick <chris@rorvick.com> wrote:
> >
> > > Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
> > > conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
> > > ("iwlwifi: dbg: move debug data to a struct") but does not implement the
> > > check correctly.
> > >
> > > This can happen in OOM situations and, when it does, we will potentially try to
> > > dereference a NULL pointer.
> > >
> > > Tweeted-by: @grsecurity
> > > Signed-off-by: Chris Rorvick <chris@rorvick.com>
> >
> > Fails to build, please rebase on top of wireless-drivers.
> >
> > drivers/net/wireless/intel/iwlwifi/iwl-drv.c: In function 'iwl_req_fw_callback':
> > drivers/net/wireless/intel/iwlwifi/iwl-drv.c:1470:16: error: 'struct iwl_fw' has no member named 'dbg_conf_tlv'
> >     if (!drv->fw.dbg_conf_tlv[i])
> >                 ^
> > make[5]: *** [drivers/net/wireless/intel/iwlwifi/iwl-drv.o] Error 1
> > make[5]: *** Waiting for unfinished jobs....
> > make[4]: *** [drivers/net/wireless/intel/iwlwifi] Error 2
> > make[3]: *** [drivers/net/wireless/intel] Error 2
> > make[2]: *** [drivers/net/wireless] Error 2
> > make[1]: *** [drivers/net] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > make: *** [drivers] Error 2
> >
>
> Should be:
>
> $ git diff
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index 0481796f75bc..c24350222133 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> @@ -1467,7 +1467,7 @@ static void iwl_req_fw_callback(const struct
> firmware *ucode_raw, void *context)
>                                 kmemdup(pieces->dbg_conf_tlv[i],
>                                         pieces->dbg_conf_tlv_len[i],
>                                         GFP_KERNEL);

Maybe this diff is clearer:

$ diff iwlwifi-actually-check-allocated-conf_tlv-pointer.patch
iwlwifi-actually-check-allocated-conf_tlv-pointer-v2-dileks.patch
95a96
> Fixes: 71bc0334a637 ("iwlwifi: check allocated pointer when allocating conf_tlvs")
99c100,104
< In this wasn't picked up?
---
>
> [ v1->v2:
>   - Fix typo s/fw.dbg_conf_tlv/fw.dbg.conf_tlv
>   - Add Fixes tag as suggested by Kalle
> -dileks ]
115c120
< +                     if (!drv->fw.dbg_conf_tlv[i])
---
> +                     if (!drv->fw.dbg.conf_tlv[i])

Tested on top of Linux v5.6.3.

- Sedat -

Re: [PATCH] iwlwifi: actually check allocated conf_tlv pointer

[Sedat Dilek]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

On Sun, Apr 5, 2020 at 11:14 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Luca Coelho <luca@coelho.fi> writes:
>
> > On Sun, 2020-04-05 at 11:44 +0300, Kalle Valo wrote:
> >> Chris Rorvick <chris@rorvick.com> writes:
> >>
> >> > Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
> >> > conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
> >> > ("iwlwifi: dbg: move debug data to a struct") but does not implement the
> >> > check correctly.
> >> >
> >> > Tweeted-by: @grsecurity
> >> > Signed-off-by: Chris Rorvick <chris@rorvick.com>
> >>
> >> I'll add:
> >>
> >> Fixes: 71bc0334a637 ("iwlwifi: check allocated pointer when allocating conf_tlvs")
> >>
> >> > ---
> >> > In this wasn't picked up?
> >>
> >> Luca, can I take this directly?
> >
> > Yes, please take it directly.
>
> Ok, assigned it to me in patchwork.
>
> > This can happen in OOM situations and, when it does, we will
> > potentially try to dereference a NULL pointer.
>
> I'll add this to the commit log.
>

Hi,

Friendly ping.

Any progress on this?

This patch seems not have landed in Linux v5.7-rc1.

$ head -5 Makefile
# SPDX-License-Identifier: GPL-2.0
VERSION = 5
PATCHLEVEL = 7
SUBLEVEL = 0
EXTRAVERSION = -rc1

$ LC_ALL=C git apply --check --verbose
../patches/iwlwifi-fixes-5.6/iwlwifi-actually-check-allocated-conf_tlv-pointer-v2-dileks.patch
Checking patch drivers/net/wireless/intel/iwlwifi/iwl-drv.c...

I have attached my v2 which I have tested on top of Linux v5.6.3.

Feel free to add my...

Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

Regards,
- Sedat -

[-- Attachment #2: iwlwifi-actually-check-allocated-conf_tlv-pointer-v2-dileks.patch --]
[-- Type: text/x-patch, Size: 5913 bytes --]

From patchwork Thu Apr  2 05:02:19 2020
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
X-Patchwork-Submitter: Chris Rorvick <chris@rorvick.com>
X-Patchwork-Id: 11470125
X-Patchwork-Delegate: kvalo@adurom.com
Return-Path: <SRS0=piom=5S=vger.kernel.org=linux-wireless-owner@kernel.org>
Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org
 [172.30.200.123])
	by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4918115AB
	for <patchwork-linux-wireless@patchwork.kernel.org>;
 Thu,  2 Apr 2020 05:40:22 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [209.132.180.67])
	by mail.kernel.org (Postfix) with ESMTP id 26CF720784
	for <patchwork-linux-wireless@patchwork.kernel.org>;
 Thu,  2 Apr 2020 05:40:22 +0000 (UTC)
Authentication-Results: mail.kernel.org;
	dkim=fail reason="signature verification failed" (2048-bit key)
 header.d=gmail.com header.i=@gmail.com header.b="oGgRiCyG"
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1727746AbgDBFkR (ORCPT
        <rfc822;patchwork-linux-wireless@patchwork.kernel.org>);
        Thu, 2 Apr 2020 01:40:17 -0400
Received: from mail-qk1-f193.google.com ([209.85.222.193]:34234 "EHLO
        mail-qk1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726201AbgDBFkR (ORCPT
        <rfc822;linux-wireless@vger.kernel.org>);
        Thu, 2 Apr 2020 01:40:17 -0400
Received: by mail-qk1-f193.google.com with SMTP id i6so2814149qke.1;
        Wed, 01 Apr 2020 22:40:16 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=20161025;
        h=sender:from:to:cc:subject:date:message-id:mime-version
         :content-transfer-encoding;
        bh=Hj9hqNcME2oIKIw+5Vx3xGDXGbaFZ4ovP+thufGAVmU=;
        b=oGgRiCyGM1NttzIOzX4lwfWaLVWlVNJVxi3p2J32f8r5BrPNarW8vx/Yl6iLZg5HU/
         y1n7ZlChgcMzf6CBkZqbMoDBLXJcQzKXki+ZUF7JoXFN4UXXS8qJ4XMG1Dy5hJIP3D2q
         PxIWu5Jz96mjpP+jTJdf8y6ohWWqVQYGRE+1Buh1xTTha5aNmdttUQo7vhVy+mQC//Xs
         LEuGp+m+fJGeAswwrLaJzN9iuSErM4LRexHOGPl21AVS1fxbUL0yQWNG/9NYEI/wx1B6
         sLOCt5cU4xBWmc99Zmu9lpWLpu4MQRIbmJsSeB8vI+u6Zhyi9W1GAygUEPh5rP6Dw1iu
         YEvw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20161025;
        h=x-gm-message-state:sender:from:to:cc:subject:date:message-id
         :mime-version:content-transfer-encoding;
        bh=Hj9hqNcME2oIKIw+5Vx3xGDXGbaFZ4ovP+thufGAVmU=;
        b=Smif+8RHLKd1nWQ1GDTERfSIhNR7k4Z7JHxwVtF7WlQghN6KbKR3gkPWuGkSbacT3/
         4DYz127WT4bSInQrSv+qtk+X12RjcBiMlovv5pHNelKD6i1a1aP9xZFo0LTwT860HKx1
         y6qxIHvObx/GheS4j0u3ogus0uNzEvTnlIptNQhA3ifwvQNbDh8CuJWaeQVvNB29si1v
         7kNV8ivhLPLpGA99IAjgA08wgROQD68QC/SpEahxG2LzEG0dITRMUeFoHAq/5YYBuBy5
         8OxEfsR0G3SqdqLYl8qnuRFFpixh++N2XKpDhqP23Y25U3UZfVRYixxpVTGCqJhqxKWQ
         FEQA==
X-Gm-Message-State: AGi0PubsIirzpTp/gtfpoocLpGEqUHWZ6H4oC2JT+fOsEwWJEP7mCPKO
        GpUyv8R62QuZ9JFioWVUj+u9Yktv
X-Google-Smtp-Source: 
 APiQypKYQzwTCauSCZsNzFwP7pSAbf43FjGqHiBn/lCnEhAbO+zIU1RyY8RLX9MuztxrJHSKsfzyeg==
X-Received: by 2002:a37:4c4d:: with SMTP id z74mr1842987qka.53.1585806015666;
        Wed, 01 Apr 2020 22:40:15 -0700 (PDT)
Received: from localhost (c-73-74-7-9.hsd1.il.comcast.net. [73.74.7.9])
        by smtp.gmail.com with ESMTPSA id
 t140sm2911459qke.48.2020.04.01.22.40.13
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Wed, 01 Apr 2020 22:40:14 -0700 (PDT)
Received: from localhost (localhost [127.0.0.1])
        by localhost (8.15.2/8.14.9) with ESMTP id 0325eAOc005904;
        Thu, 2 Apr 2020 00:40:12 -0500
Received: (from chris@localhost)
        by localhost (8.15.2/8.15.2/Submit) id 03254KY3004887;
        Thu, 2 Apr 2020 00:04:20 -0500
From: Chris Rorvick <chris@rorvick.com>
To: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc: Chris Rorvick <chris@rorvick.com>,
        Johannes Berg <johannes.berg@intel.com>,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Luca Coelho <luciano.coelho@intel.com>,
        Intel Linux Wireless <linuxwifi@intel.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>
Subject: [PATCH] iwlwifi: actually check allocated conf_tlv pointer
Date: Thu,  2 Apr 2020 00:02:19 -0500
Message-Id: <20200402050219.4842-1-chris@rorvick.com>
X-Mailer: git-send-email 2.25.0
MIME-Version: 1.0
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

Commit 71bc0334a637 ("iwlwifi: check allocated pointer when allocating
conf_tlvs") attempted to fix a typoe introduced by commit 17b809c9b22e
("iwlwifi: dbg: move debug data to a struct") but does not implement the
check correctly.

Fixes: 71bc0334a637 ("iwlwifi: check allocated pointer when allocating conf_tlvs")
Tweeted-by: @grsecurity
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---

[ v1->v2:
  - Fix typo s/fw.dbg_conf_tlv/fw.dbg.conf_tlv
  - Add Fixes tag as suggested by Kalle
-dileks ]

 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index ff52e69c1c80..a37f330e7bd4 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1465,11 +1465,11 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
 		if (pieces->dbg_conf_tlv[i]) {
 			drv->fw.dbg.conf_tlv[i] =
 				kmemdup(pieces->dbg_conf_tlv[i],
 					pieces->dbg_conf_tlv_len[i],
 					GFP_KERNEL);
-			if (!pieces->dbg_conf_tlv[i])
+			if (!drv->fw.dbg.conf_tlv[i])
 				goto out_free_fw;
 		}
 	}
 
 	memset(&trigger_tlv_sz, 0xff, sizeof(trigger_tlv_sz));