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

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

[Suren Baghdasaryan]

When handling SHDLC I-Frame commands "pipe" field used for indexing
into an array should be checked before usage. If left unchecked it
might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).

Malformed NFC HCI frames could be injected by a malicious NFC device
communicating with the device being attacked (remote attack vector),
or even by an attacker with physical access to the I2C bus such that
they could influence the data transfers on that bus (local attack vector).
skb->data is controlled by the attacker and has only been sanitized in
the most trivial ways (CRC check), therefore we can consider the
create_info struct and all of its members to tainted. 'create_info->pipe'
with max value of 255 (uint8) is used to take an offset of the
hdev->pipes array of 127 elements which can lead to OOB write.

Suggested-by: Kevin Deus <kdeus@google.com>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 net/nfc/hci/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
index ac8030c4bcf8..19cb2e473ea6 100644
--- a/net/nfc/hci/core.c
+++ b/net/nfc/hci/core.c
@@ -209,6 +209,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
 		}
 		create_info = (struct hci_create_pipe_resp *)skb->data;
 
+		if (create_info->pipe >= NFC_HCI_MAX_PIPES) {
+			status = NFC_HCI_ANY_E_NOK;
+			goto exit;
+		}
+
 		/* Save the new created pipe and bind with local gate,
 		 * the description for skb->data[3] is destination gate id
 		 * but since we received this cmd from host controller, we
@@ -232,6 +237,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
 		}
 		delete_info = (struct hci_delete_pipe_noti *)skb->data;
 
+		if (delete_info->pipe >= NFC_HCI_MAX_PIPES) {
+			status = NFC_HCI_ANY_E_NOK;
+			goto exit;
+		}
+
 		hdev->pipes[delete_info->pipe].gate = NFC_HCI_INVALID_GATE;
 		hdev->pipes[delete_info->pipe].dest_host = NFC_HCI_INVALID_HOST;
 		break;
-- 
2.18.0.597.ga71716f1ad-goog

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

[Kees Cook]

On Mon, Aug 13, 2018 at 3:39 PM, Suren Baghdasaryan <surenb@google.com> wrote:
> When handling SHDLC I-Frame commands "pipe" field used for indexing
> into an array should be checked before usage. If left unchecked it
> might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).
>
> Malformed NFC HCI frames could be injected by a malicious NFC device
> communicating with the device being attacked (remote attack vector),
> or even by an attacker with physical access to the I2C bus such that
> they could influence the data transfers on that bus (local attack vector).
> skb->data is controlled by the attacker and has only been sanitized in
> the most trivial ways (CRC check), therefore we can consider the
> create_info struct and all of its members to tainted. 'create_info->pipe'
> with max value of 255 (uint8) is used to take an offset of the
> hdev->pipes array of 127 elements which can lead to OOB write.
>
> Suggested-by: Kevin Deus <kdeus@google.com>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Nice find!

Acked-by: Kees Cook <keescook@chromium.org>

> ---
>  net/nfc/hci/core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
> index ac8030c4bcf8..19cb2e473ea6 100644
> --- a/net/nfc/hci/core.c
> +++ b/net/nfc/hci/core.c
> @@ -209,6 +209,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
>                 }
>                 create_info = (struct hci_create_pipe_resp *)skb->data;
>
> +               if (create_info->pipe >= NFC_HCI_MAX_PIPES) {
> +                       status = NFC_HCI_ANY_E_NOK;
> +                       goto exit;
> +               }
> +
>                 /* Save the new created pipe and bind with local gate,
>                  * the description for skb->data[3] is destination gate id
>                  * but since we received this cmd from host controller, we
> @@ -232,6 +237,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
>                 }
>                 delete_info = (struct hci_delete_pipe_noti *)skb->data;
>
> +               if (delete_info->pipe >= NFC_HCI_MAX_PIPES) {
> +                       status = NFC_HCI_ANY_E_NOK;
> +                       goto exit;
> +               }
> +
>                 hdev->pipes[delete_info->pipe].gate = NFC_HCI_INVALID_GATE;
>                 hdev->pipes[delete_info->pipe].dest_host = NFC_HCI_INVALID_HOST;
>                 break;
> --
> 2.18.0.597.ga71716f1ad-goog
>



-- 
Kees Cook
Pixel Security

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

[Greg KH]

On Mon, Aug 13, 2018 at 03:39:08PM -0700, Suren Baghdasaryan wrote:
> When handling SHDLC I-Frame commands "pipe" field used for indexing
> into an array should be checked before usage. If left unchecked it
> might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).
> 
> Malformed NFC HCI frames could be injected by a malicious NFC device
> communicating with the device being attacked (remote attack vector),
> or even by an attacker with physical access to the I2C bus such that
> they could influence the data transfers on that bus (local attack vector).
> skb->data is controlled by the attacker and has only been sanitized in
> the most trivial ways (CRC check), therefore we can consider the
> create_info struct and all of its members to tainted. 'create_info->pipe'
> with max value of 255 (uint8) is used to take an offset of the
> hdev->pipes array of 127 elements which can lead to OOB write.
> 
> Suggested-by: Kevin Deus <kdeus@google.com>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Suren Baghdasaryan]

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?

>
> regards,
> dan carpenter
>

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

[Suren Baghdasaryan]

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.

>>
>> regards,
>> dan carpenter
>>

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

[Kees Cook]

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.

-Kees

-- 
Kees Cook
Pixel Security

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

[Suren Baghdasaryan]

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!

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

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

[Kees Cook]

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

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

[Suren Baghdasaryan]

On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook <keescook@chromium.org> wrote:
> 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?
>>>>>

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.

>>>>
>>>> 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

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

[Suren Baghdasaryan]

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 <surenb@google.com> wrote:
> On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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?
>>>>>>
>
> 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.
>
>>>>>
>>>>> 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

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

[Dan Carpenter]

On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote:
> The separate fix for the size of pipes[] array is posted here:
> https://lkml.org/lkml/2018/8/14/1034
> Thanks!
> 

That's great!  Let's add some bounds checking to nfc_hci_msg_rx_work()
and nfc_hci_recv_from_llc() as well and then we can close the chapter on
these bugs.

regards,
dan carpenter

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

[Suren Baghdasaryan]

On Wed, Aug 15, 2018 at 1:29 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote:
>> The separate fix for the size of pipes[] array is posted here:
>> https://lkml.org/lkml/2018/8/14/1034
>> Thanks!
>>
>
> That's great!  Let's add some bounds checking to nfc_hci_msg_rx_work()
> and nfc_hci_recv_from_llc() as well and then we can close the chapter on
> these bugs.

Dan, I don't think we need additional checks there. Here are the
relevant parts of the code in nfc_hci_recv_from_llc():

static void nfc_hci_recv_from_llc(struct nfc_hci_dev *hdev, struct sk_buff *skb)
{
...
packet = (struct hcp_packet *)skb->data;
...

/* it's the last fragment. Does it need re-aggregation? */
if (skb_queue_len(&hdev->rx_hcp_frags)) {
    pipe = packet->header & NFC_HCI_FRAGMENT;
    ...
    hcp_skb = nfc_alloc_recv_skb(NFC_HCI_HCP_PACKET_HEADER_LEN +
         msg_len, GFP_KERNEL);
    ...
    *skb_put(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN) = pipe;
    ...
} else {
    packet->header &= NFC_HCI_FRAGMENT;
    hcp_skb = skb;
}

AFAIU in both cases the pipe field in hcp_skb can't exceed 127 after
we applied NFC_HCI_FRAGMENT(0x7f) mask.

>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

On Wed, Aug 15, 2018 at 09:40:13AM -0700, Suren Baghdasaryan wrote:
> On Wed, Aug 15, 2018 at 1:29 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote:
> >> The separate fix for the size of pipes[] array is posted here:
> >> https://lkml.org/lkml/2018/8/14/1034
> >> Thanks!
> >>
> >
> > That's great!  Let's add some bounds checking to nfc_hci_msg_rx_work()
> > and nfc_hci_recv_from_llc() as well and then we can close the chapter on
> > these bugs.
> 
> Dan, I don't think we need additional checks there. Here are the
> relevant parts of the code in nfc_hci_recv_from_llc():
>

Sorry, I meant after that at the end of the function:

net/nfc/hci/core.c
   902          /* if this is a response, dispatch immediately to
   903           * unblock waiting cmd context. Otherwise, enqueue to dispatch
   904           * in separate context where handler can also execute command.
   905           */
   906          packet = (struct hcp_packet *)hcp_skb->data;
   907          type = HCP_MSG_GET_TYPE(packet->message.header);
   908          if (type == NFC_HCI_HCP_RESPONSE) {
   909                  pipe = packet->header;
                        ^^^^^^^^^^^^^^^^^^^^^
Pipe can go up to 255.

   910                  instruction = HCP_MSG_GET_CMD(packet->message.header);
   911                  skb_pull(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN +
   912                           NFC_HCI_HCP_MESSAGE_HEADER_LEN);
   913                  nfc_hci_hcp_message_rx(hdev, pipe, type, instruction, hcp_skb);
                                                     ^^^^
Then inside the nfc_hci_hcp_message_rx() function we call
nfc_hci_cmd_received() and nfc_hci_event_received() which use it as an
array index.

   914          } else {
   915                  skb_queue_tail(&hdev->msg_rx_queue, hcp_skb);
   916                  schedule_work(&hdev->msg_rx_work);
   917          }
   918  }

It's the same thing when nfc_hci_hcp_message_rx() is called from
nfc_hci_msg_rx_work():

   138  static void nfc_hci_msg_rx_work(struct work_struct *work)
   139  {
   140          struct nfc_hci_dev *hdev = container_of(work, struct nfc_hci_dev,
   141                                                  msg_rx_work);
   142          struct sk_buff *skb;
   143          struct hcp_message *message;
   144          u8 pipe;
   145          u8 type;
   146          u8 instruction;
   147  
   148          while ((skb = skb_dequeue(&hdev->msg_rx_queue)) != NULL) {
   149                  pipe = skb->data[0];
                        ^^^^^^^^^^^^^^^^^^^
   150                  skb_pull(skb, NFC_HCI_HCP_PACKET_HEADER_LEN);
   151                  message = (struct hcp_message *)skb->data;
   152                  type = HCP_MSG_GET_TYPE(message->header);
   153                  instruction = HCP_MSG_GET_CMD(message->header);
   154                  skb_pull(skb, NFC_HCI_HCP_MESSAGE_HEADER_LEN);
   155  
   156                  nfc_hci_hcp_message_rx(hdev, pipe, type, instruction, skb);
                                                     ^^^^
   157          }
   158  }

regards,
dan carpenter

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

[Dan Carpenter]

Never mind.  This code is a bit subtle for static analysis so those
warnings are false positives.  Thanks for taking a look at this, Suren.

regards,
dan carpenter