lkml.org 
[lkml]   [2018]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands
    On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan <surenb@google.com> wrote:
    > On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote:
    >> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote:
    >>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote:
    >>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.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!

    If it's already landed separately somewhere else, then yeah, 2 patches
    sounds good. No objection either way from me!

    -Kees

    --
    Kees Cook
    Pixel Security

    \
     
     \ /
      Last update: 2018-08-14 23:51    [W:2.431 / U:0.828 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site