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

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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