Re: [PATCH 2/2] Bluetooth: hci_uart: Fix dereferencing of ERR_PTR

[Chan-yeol Park <chanyeol.park@samsung.com>]

Hi Marcel,

On 06/16/2015 11:29 PM, Marcel Holtmann wrote:
> Hi Chan-yeol,
>
>> If h4_recv() return ERR_PTR instead sk_buff pointer, it should be
>> cleared once dereference is completed for the further reference such as
>> h4_recv(), or h4_close().
>
> I have no idea what the h4_close has to do with it? Can you explain.
If h4->rx_skb has ERR_PTR , kfree_skb() would dereference of ERR_PTR. 
This is easily reproduced on my board when I turn off BT.
>
>>
>> Signed-off-by: Chan-yeol Park <chanyeol.park@samsung.com>
>> ---
>> drivers/bluetooth/hci_h4.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
>> index f7190f0..a8acd99 100644
>> --- a/drivers/bluetooth/hci_h4.c
>> +++ b/drivers/bluetooth/hci_h4.c
>> @@ -133,6 +133,7 @@ static int h4_recv(struct hci_uart *hu, const void *data, int count)
>> 	if (IS_ERR(h4->rx_skb)) {
>> 		int err = PTR_ERR(h4->rx_skb);
>> 		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
>> +		h4->rx_skb = NULL;
>> 		return err;
>> 	}
>
> Isn't this better fixed in h4_recv_buf directly:
Yes it's better. I think if we use ERR_PTR this would be right.
>
> @@ -173,7 +173,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
>          while (count) {
>                  int i, len;
>
> -               if (!skb) {
> +               if (IS_ERR_OR_NULL(skb)) {
>                          for (i = 0; i < pkts_count; i++) {
>                                  if (buffer[0] != (&pkts[i])->type)
>                                          continue;
>
>>
>> @@ -248,6 +249,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
>> 				break;
>> 			default:
>> 				/* Unsupported variable length */
>> +
>
> This change seems totally unrelated.
Sorry, Unexpectedly this blank line is added.
>
>> 				kfree_skb(skb);
>> 				return ERR_PTR(-EILSEQ);
>> 			}
>
> Regards
>
> Marcel
>
>
I would raise v2.

Thanks
Chanyeol