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

The separate fix for the size of pipes[] array is posted here:
https://lkml.org/lkml/2018/8/14/1034
Thanks!

On Tue, Aug 14, 2018 at 3:19 PM, Suren Baghdasaryan [off-list ref] wrote:

On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

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?

Actually, after looking more closely, NFC_HCI_MAX_PIPES is always used
as the number of supported pipes and not as the max pipe ID, so the
right fix would be:

-#define NFC_HCI_MAX_PIPES 127
+#define NFC_HCI_MAX_PIPES 128

I would prefer to rename it into NFC_HCI_PIPE_COUNT but don't want to
introduce unnecessary churn for one-line change, so will keep the
name. Will post a separate fix for this shortly.

quoted

quoted

quoted

quoted

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