netdev - Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Aug 2018 13:55:23 -0700
From:   Suren Baghdasaryan <surenb@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        Security Officers <security@...nel.org>,
        Kevin Deus <kdeus@...gle.com>,
        Samuel Ortiz <sameo@...ux.intel.com>,
        "David S. Miller" <davem@...emloft.net>,
        Allen Pais <allen.pais@...cle.com>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling
 SHDLC I-Frame commands

On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@...omium.org> wrote:
> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@...gle.com> wrote:
>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@...gle.com> wrote:
>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
>>>> Thanks.  This is great.  I'm so glad these are finally getting fixed.
>>>>
>>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
>>>> well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
>>>> (0x7f) so that's one element beyond the end of the array and the
>>>> NFC_HCI_HCP_RESPONSE isn't checked.
>>>>
>>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
>>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one.
>>>
>>> Good point. From hci.h:
>>>
>>> /*
>>>  * According to specification 102 622 chapter 4.4 Pipes,
>>>  * the pipe identifier is 7 bits long.
>>>  */
>>> #define NFC_HCI_MAX_PIPES 127
>>>
>>> And then:
>>>
>>> struct nfc_hci_dev {
>>>   ...
>>>   struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
>>>   ...
>>> }
>>>
>>> I think the correct fix would be to change it to:
>>>
>>>   struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];
>>>
>>> What do you think?
>>>
>>
>> Just to be clear, this would fix the problem Dan described in his
>> reply and it should be implemented in a separate patch. The original
>> fix is still valid.
>
> I think you could merge the fixes into a single patch.

Couple reasons I would prefer to keep them separate:
- I feel that descriptions for these two issues should be different
and it's easier if we don't mix them up
- This one is already merged into Android kernels, so would be easier
to introduce the second fix separately
- I would like to give credit to people who noticed the problem (in
this case those are different people)

However if more people think we should fix both issues in the same
patch I'll happily do that.
Thanks!

>
> -Kees
>
> --
> Kees Cook
> Pixel Security