re: NFC: HCI support

re: NFC: HCI support

[Dan Carpenter]

Hello Eric Lapuyade,

This is a semi-automatic email about new static checker warnings.

The patch 8b8d2e08bf0d: "NFC: HCI support" from Apr 10, 2012, leads 
to the following Smatch complaint:

net/nfc/hci/core.c:815 nfc_hci_recv_frame()
	 error: we previously assumed 'hcp_skb' could be null (see line 790)

net/nfc/hci/core.c
   789						     msg_len, GFP_KERNEL);
   790			if (hcp_skb == NULL) {
                            ^^^^^^^^^^^^^^^
New check.

   791				/* TODO ELa: cannot deliver HCP message. How to
   792				 * propagate error up?
   793				 */

If we can't allocate memory here, shouldn't we just drop the packet and
return?  The other layers can handle dropped packets.

   794			}
   795	
   796			*skb_put(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN) = pipe;
   797	
   798			skb_queue_walk(&hdev->rx_hcp_frags, frag_skb) {
   799				msg_len = frag_skb->len - NFC_HCI_HCP_PACKET_HEADER_LEN;
   800				memcpy(skb_put(hcp_skb, msg_len),
   801				       frag_skb->data + NFC_HCI_HCP_PACKET_HEADER_LEN,
   802				       msg_len);
   803			}
   804	
   805			skb_queue_purge(&hdev->rx_hcp_frags);
   806		} else {
   807			packet->header &= NFC_HCI_FRAGMENT;
   808			hcp_skb = skb;
   809		}
   810	
   811		/* if this is a response, dispatch immediately to
   812		 * unblock waiting cmd context. Otherwise, enqueue to dispatch
   813		 * in separate context where handler can also execute command.
   814		 */
   815		packet = (struct hcp_packet *)hcp_skb->data;
                                              ^^^^^^^^^^^^^
New dereference.

   816		type = HCP_MSG_GET_TYPE(packet->message.header);
   817		if (type == NFC_HCI_HCP_RESPONSE) {

regards,
dan carpenter

Re: NFC: HCI support

[Eric Lapuyade]

Hi Dan,

Thank you for the report. Please see below my explanations.

Regards,

Eric

On 13/04/2012 12:16, Dan Carpenter wrote:
> Hello Eric Lapuyade,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 8b8d2e08bf0d: "NFC: HCI support" from Apr 10, 2012, leads
> to the following Smatch complaint:
>
> net/nfc/hci/core.c:815 nfc_hci_recv_frame()
> 	 error: we previously assumed 'hcp_skb' could be null (see line 790)
>
> net/nfc/hci/core.c
>     789						     msg_len, GFP_KERNEL);
>     790			if (hcp_skb == NULL) {
>                              ^^^^^^^^^^^^^^^
> New check.
>
>     791				/* TODO ELa: cannot deliver HCP message. How to
>     792				 * propagate error up?
>     793				 */
>
> If we can't allocate memory here, shouldn't we just drop the packet and
> return?  The other layers can handle dropped packets.

Actually, the higher level to which this frame is destined cannot handle lost packets. In the current case, the frame can contain a response to an outgoing command, or an event or a command from the chipset.
In case of the response, the upper layer would complete command execution with a timeout error. So basically it would work except the command would complete with an incorrect error indication which could be a problem if you try to understand why the chip didn't answer your command.
In case of an event, that could be a tag_discovered event for example. Events MUST be delivered, otherwise it would appear to the user like if the tag wasn't detected at all, with no indication that something failed.
In case of an incoming command, it would be no problem, the remote would timeout on our response.

I will soon commit a new patch to purge the fragment list and return. This will remove the checker errors. I'll leave the TODO until we have designed a way for upper layer to be notified.

>
>     794			}
>     795	
>     796			*skb_put(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN) = pipe;
>     797	
>     798			skb_queue_walk(&hdev->rx_hcp_frags, frag_skb) {
>     799				msg_len = frag_skb->len - NFC_HCI_HCP_PACKET_HEADER_LEN;
>     800				memcpy(skb_put(hcp_skb, msg_len),
>     801				       frag_skb->data + NFC_HCI_HCP_PACKET_HEADER_LEN,
>     802				       msg_len);
>     803			}
>     804	
>     805			skb_queue_purge(&hdev->rx_hcp_frags);
>     806		} else {
>     807			packet->header&= NFC_HCI_FRAGMENT;
>     808			hcp_skb = skb;
>     809		}
>     810	
>     811		/* if this is a response, dispatch immediately to
>     812		 * unblock waiting cmd context. Otherwise, enqueue to dispatch
>     813		 * in separate context where handler can also execute command.
>     814		 */
>     815		packet = (struct hcp_packet *)hcp_skb->data;
>                                                ^^^^^^^^^^^^^
> New dereference.
>
>     816		type = HCP_MSG_GET_TYPE(packet->message.header);
>     817		if (type == NFC_HCI_HCP_RESPONSE) {
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html