In h4_recv(), if h4_recv_buf() returns error and h4_recv() is
asynchronously called again before setting rx_skb to NULL, ERR_PTR will
be dereferenced in h4_recv_buf(). Check return value in a local variable
before writing to rx_skb.
Reported-by: [email protected]
Signed-off-by: Myungho Jung <[email protected]>
---
drivers/bluetooth/hci_h4.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index fb97a3bf069b..fa30ec9cebd4 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -124,19 +124,22 @@ static const struct h4_recv_pkt h4_recv_pkts[] = {
static int h4_recv(struct hci_uart *hu, const void *data, int count)
{
struct h4_struct *h4 = hu->priv;
+ struct sk_buff *skb;
if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
return -EUNATCH;
- h4->rx_skb = h4_recv_buf(hu->hdev, h4->rx_skb, data, count,
- h4_recv_pkts, ARRAY_SIZE(h4_recv_pkts));
- if (IS_ERR(h4->rx_skb)) {
- int err = PTR_ERR(h4->rx_skb);
+ skb = h4_recv_buf(hu->hdev, h4->rx_skb, data, count,
+ h4_recv_pkts, ARRAY_SIZE(h4_recv_pkts));
+ if (IS_ERR(skb)) {
+ int err = PTR_ERR(skb);
bt_dev_err(hu->hdev, "Frame reassembly failed (%d)", err);
h4->rx_skb = NULL;
return err;
}
+ h4->rx_skb = skb;
+
return count;
}
--
2.17.1
Hi Myungho,
> In h4_recv(), if h4_recv_buf() returns error and h4_recv() is
> asynchronously called again before setting rx_skb to NULL, ERR_PTR will
> be dereferenced in h4_recv_buf(). Check return value in a local variable
> before writing to rx_skb.
>
> Reported-by: [email protected]
> Signed-off-by: Myungho Jung <[email protected]>
> ---
> drivers/bluetooth/hci_h4.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
patch has been applied to bluetooth-next tree.
Can you actually fix all callers of h4_recv_buf since they all suffer from the same issue.
Regards
Marcel
On Fri, Jan 18, 2019 at 10:19:41AM +0100, Marcel Holtmann wrote:
> Hi Myungho,
>
> > In h4_recv(), if h4_recv_buf() returns error and h4_recv() is
> > asynchronously called again before setting rx_skb to NULL, ERR_PTR will
> > be dereferenced in h4_recv_buf(). Check return value in a local variable
> > before writing to rx_skb.
> >
> > Reported-by: [email protected]
> > Signed-off-by: Myungho Jung <[email protected]>
> > ---
> > drivers/bluetooth/hci_h4.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
>
> patch has been applied to bluetooth-next tree.
>
> Can you actually fix all callers of h4_recv_buf since they all suffer from the same issue.
>
> Regards
>
> Marcel
>
Hi Marcel,
Sure, let me check other callers and fix them if applicable.
Thanks,
Myungho
On Sat, Jan 19, 2019 at 12:19:20AM -0800, Myungho Jung wrote:
> On Fri, Jan 18, 2019 at 10:19:41AM +0100, Marcel Holtmann wrote:
> > Hi Myungho,
> >
> > > In h4_recv(), if h4_recv_buf() returns error and h4_recv() is
> > > asynchronously called again before setting rx_skb to NULL, ERR_PTR will
> > > be dereferenced in h4_recv_buf(). Check return value in a local variable
> > > before writing to rx_skb.
> > >
> > > Reported-by: [email protected]
> > > Signed-off-by: Myungho Jung <[email protected]>
> > > ---
> > > drivers/bluetooth/hci_h4.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > patch has been applied to bluetooth-next tree.
> >
> > Can you actually fix all callers of h4_recv_buf since they all suffer from the same issue.
> >
> > Regards
> >
> > Marcel
> >
>
> Hi Marcel,
>
> Sure, let me check other callers and fix them if applicable.
>
> Thanks,
> Myungho
>
Hi Marcel,
I found there are many callers that need to be fixed. So, how about checking
error code in h4_recv_buf() instead?
diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index fb97a3bf069b..dea48090d2dc 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -174,6 +174,10 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
struct hci_uart *hu = hci_get_drvdata(hdev);
u8 alignment = hu->alignment ? hu->alignment : 1;
+ /* Check if socket buffer is not reset yet from previous error */
+ if (IS_ERR(skb))
+ skb = NULL;
+
while (count) {
int i, len;
It is tested and verified by syzbot. The previous commit is no more needed if
this looks better.
Thanks,
Myungho
Hi Myungho,
>>>> In h4_recv(), if h4_recv_buf() returns error and h4_recv() is
>>>> asynchronously called again before setting rx_skb to NULL, ERR_PTR will
>>>> be dereferenced in h4_recv_buf(). Check return value in a local variable
>>>> before writing to rx_skb.
>>>>
>>>> Reported-by: [email protected]
>>>> Signed-off-by: Myungho Jung <[email protected]>
>>>> ---
>>>> drivers/bluetooth/hci_h4.c | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> patch has been applied to bluetooth-next tree.
>>>
>>> Can you actually fix all callers of h4_recv_buf since they all suffer from the same issue.
>>>
>>> Regards
>>>
>>> Marcel
>>>
>>
>> Hi Marcel,
>>
>> Sure, let me check other callers and fix them if applicable.
>>
>> Thanks,
>> Myungho
>>
>
> Hi Marcel,
>
> I found there are many callers that need to be fixed. So, how about checking
> error code in h4_recv_buf() instead?
>
> diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
> index fb97a3bf069b..dea48090d2dc 100644
> --- a/drivers/bluetooth/hci_h4.c
> +++ b/drivers/bluetooth/hci_h4.c
> @@ -174,6 +174,10 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
> struct hci_uart *hu = hci_get_drvdata(hdev);
> u8 alignment = hu->alignment ? hu->alignment : 1;
>
> + /* Check if socket buffer is not reset yet from previous error */
> + if (IS_ERR(skb))
> + skb = NULL;
> +
> while (count) {
> int i, len;
>
>
> It is tested and verified by syzbot. The previous commit is no more needed if
> this looks better.
please send a proper patch for this and also don’t forget drivers/bluetooth/h4_recv.h since these two are not yet consolidated.
Regards
Marcel