Add the check for the return value of skb_clone since it may return NULL
pointer and cause NULL pointer dereference in send_pkt.
Fixes: 18722c247023 ("Bluetooth: Enable 6LoWPAN support for BT LE devices")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
Changelog:
v1 -> v2:
1. Modify the error handling in the loop.
---
net/bluetooth/6lowpan.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 4eb1b3ced0d2..55ae2ff40efb 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -477,19 +477,25 @@ static int send_mcast_pkt(struct sk_buff *skb, struct net_device *netdev)
int ret;
local_skb = skb_clone(skb, GFP_ATOMIC);
+ if (!local_skb) {
+ err = -ENOMEM;
+ goto out;
+ }
BT_DBG("xmit %s to %pMR type %u IP %pI6c chan %p",
netdev->name,
&pentry->chan->dst, pentry->chan->dst_type,
&pentry->peer_addr, pentry->chan);
ret = send_pkt(pentry->chan, local_skb, netdev);
- if (ret < 0)
- err = ret;
-
kfree_skb(local_skb);
+ if (ret < 0) {
+ err = ret;
+ goto out;
+ }
}
}
+out:
rcu_read_unlock();
return err;
--
2.25.1
On Wed, Mar 15, 2023 at 03:06:21PM +0800, Jiasheng Jiang wrote:
> Add the check for the return value of skb_clone since it may return NULL
> pointer and cause NULL pointer dereference in send_pkt.
>
> Fixes: 18722c247023 ("Bluetooth: Enable 6LoWPAN support for BT LE devices")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> Changelog:
>
> v1 -> v2:
>
> 1. Modify the error handling in the loop.
I think that at a minimum this needs to be included in the patch description.
Or better, in it's own patch with it's own fixes tag.
It seems like a fundamental change to the error handling to me.
> ---
> net/bluetooth/6lowpan.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 4eb1b3ced0d2..55ae2ff40efb 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -477,19 +477,25 @@ static int send_mcast_pkt(struct sk_buff *skb, struct net_device *netdev)
> int ret;
>
> local_skb = skb_clone(skb, GFP_ATOMIC);
> + if (!local_skb) {
> + err = -ENOMEM;
> + goto out;
> + }
>
> BT_DBG("xmit %s to %pMR type %u IP %pI6c chan %p",
> netdev->name,
> &pentry->chan->dst, pentry->chan->dst_type,
> &pentry->peer_addr, pentry->chan);
> ret = send_pkt(pentry->chan, local_skb, netdev);
> - if (ret < 0)
> - err = ret;
> -
> kfree_skb(local_skb);
> + if (ret < 0) {
> + err = ret;
> + goto out;
> + }
> }
> }
>
> +out:
> rcu_read_unlock();
>
> return err;
> --
> 2.25.1
>
On Sat, Mar 18, 2023 at 05:03:21AM +0800, Simon Horman wrote:
> On Wed, Mar 15, 2023 at 03:06:21PM +0800, Jiasheng Jiang wrote:
>> Add the check for the return value of skb_clone since it may return NULL
>> pointer and cause NULL pointer dereference in send_pkt.
>>
>> Fixes: 18722c247023 ("Bluetooth: Enable 6LoWPAN support for BT LE devices")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>> ---
>> Changelog:
>>
>> v1 -> v2:
>>
>> 1. Modify the error handling in the loop.
>
> I think that at a minimum this needs to be included in the patch description.
> Or better, in it's own patch with it's own fixes tag.
> It seems like a fundamental change to the error handling to me.
I will submit a separate patch to modify the error handling in the loop.
You can directly review the v1.
Link:https://lore.kernel.org/all/[email protected]/
Thanks,
Jiang
On Mon, Mar 20, 2023 at 10:54:40AM +0800, Jiasheng Jiang wrote:
> On Sat, Mar 18, 2023 at 05:03:21AM +0800, Simon Horman wrote:
>> On Wed, Mar 15, 2023 at 03:06:21PM +0800, Jiasheng Jiang wrote:
>>> Add the check for the return value of skb_clone since it may return NULL
>>> pointer and cause NULL pointer dereference in send_pkt.
>>>
>>> Fixes: 18722c247023 ("Bluetooth: Enable 6LoWPAN support for BT LE devices")
>>> Signed-off-by: Jiasheng Jiang <[email protected]>
>>> ---
>>> Changelog:
>>>
>>> v1 -> v2:
>>>
>>> 1. Modify the error handling in the loop.
>>
>> I think that at a minimum this needs to be included in the patch description.
>> Or better, in it's own patch with it's own fixes tag.
>> It seems like a fundamental change to the error handling to me.
>
> I will submit a separate patch to modify the error handling in the loop.
> You can directly review the v1.
> Link:https://lore.kernel.org/all/[email protected]/
I think it would be better to send a patch series.
Thanks,
Jiang
On Mon, Mar 20, 2023 at 11:08:46AM +0800, Jiasheng Jiang wrote:
> On Mon, Mar 20, 2023 at 10:54:40AM +0800, Jiasheng Jiang wrote:
> > On Sat, Mar 18, 2023 at 05:03:21AM +0800, Simon Horman wrote:
> >> On Wed, Mar 15, 2023 at 03:06:21PM +0800, Jiasheng Jiang wrote:
> >>> Add the check for the return value of skb_clone since it may return NULL
> >>> pointer and cause NULL pointer dereference in send_pkt.
> >>>
> >>> Fixes: 18722c247023 ("Bluetooth: Enable 6LoWPAN support for BT LE devices")
> >>> Signed-off-by: Jiasheng Jiang <[email protected]>
> >>> ---
> >>> Changelog:
> >>>
> >>> v1 -> v2:
> >>>
> >>> 1. Modify the error handling in the loop.
> >>
> >> I think that at a minimum this needs to be included in the patch description.
> >> Or better, in it's own patch with it's own fixes tag.
> >> It seems like a fundamental change to the error handling to me.
> >
> > I will submit a separate patch to modify the error handling in the loop.
> > You can directly review the v1.
> > Link:https://lore.kernel.org/all/[email protected]/
>
> I think it would be better to send a patch series.
Yes, agreed.
On Tue, Mar 21, 2023 at 00:09:11AM +0800, Simon Horman wrote:
>On Mon, Mar 20, 2023 at 02:31:55PM +0800, Jiasheng Jiang wrote:
>> Return the error when send_pkt fails in order to avoid the error being
>> overwritten.
>> Moreover, remove the redundant 'ret'.
>>
>> Fixes: 9c238ca8ec79 ("Bluetooth: 6lowpan: Check transmit errors for multicast packets")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>
> I see that the error handling is imperfect - only the most recent
> error value is returned.
>
> But I think this patch introduces a behavioural change: if
> an error occurs then no attempt is made to send the
> multicast packet to devices that follow in the list of peers.
>
> If so, I'd want to be sure that behaviour is desirable.
I think it's a matter of trade-offs.
The original error handling can complete the remaining correct tasks.
However, my patch can avoid resource waste, because if the an
error occurs, the rest is likely to go wrong.
For example, if a memory allocation fails because of the insufficient
memory, the next memory allocation will likely fails too.
Maybe it is better to use different error handlings depending on the
type of errors:
Immediately return "ENOMEM" errors and continue execute if the other errors occur.
Thanks,
Jiang
On Wed, Mar 29, 2023 at 10:08:10AM +0800, Jiasheng Jiang wrote:
> On Tue, Mar 21, 2023 at 00:09:11AM +0800, Simon Horman wrote:
> >On Mon, Mar 20, 2023 at 02:31:55PM +0800, Jiasheng Jiang wrote:
> >> Return the error when send_pkt fails in order to avoid the error being
> >> overwritten.
> >> Moreover, remove the redundant 'ret'.
> >>
> >> Fixes: 9c238ca8ec79 ("Bluetooth: 6lowpan: Check transmit errors for multicast packets")
> >> Signed-off-by: Jiasheng Jiang <[email protected]>
> >
> > I see that the error handling is imperfect - only the most recent
> > error value is returned.
> >
> > But I think this patch introduces a behavioural change: if
> > an error occurs then no attempt is made to send the
> > multicast packet to devices that follow in the list of peers.
> >
> > If so, I'd want to be sure that behaviour is desirable.
>
> I think it's a matter of trade-offs.
> The original error handling can complete the remaining correct tasks.
> However, my patch can avoid resource waste, because if the an
> error occurs, the rest is likely to go wrong.
> For example, if a memory allocation fails because of the insufficient
> memory, the next memory allocation will likely fails too.
I see your point.
> Maybe it is better to use different error handlings depending on the
> type of errors:
> Immediately return "ENOMEM" errors and continue execute if the other errors occur.
Yes, that might be interesting if we can clearly
differentiate between the two types of errors.
Yet, it brings complexity.
Given your explanation, perhaps the best idea is the implementation
provided by this patch.