Re: [PATCH] datapath.c: fix missing return value check of nla_nest_start()
On Tue, Aug 21, 2018 at 4:38 PM David Miller wrote:
>
> From: Pravin Shelar
> Date: Tue, 21 Aug 2018 15:38:28 -0700
>
> > On Fri, Aug 17, 2018 at 1:15 AM Jiecheng Wu wrote:
> >>
> >> Function queue_userspace_packet() defined in net/openvswitch/datapath.c
> >> calls nla_nest_start() to allocate memory for struct nlattr which is
> >> dereferenced immediately. As nla_nest_start() may return NULL on failure,
> >> this code piece may cause NULL pointer dereference bug.
> >> ---
> >> net/openvswitch/datapath.c | 4
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> >> index 0f5ce77..ff4457d 100644
> >> --- a/net/openvswitch/datapath.c
> >> +++ b/net/openvswitch/datapath.c
> >> @@ -460,6 +460,8 @@ static int queue_userspace_packet(struct datapath *dp,
> >> struct sk_buff *skb,
> >>
> >> if (upcall_info->egress_tun_info) {
> >> nla = nla_nest_start(user_skb,
> >> OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> >> + if (!nla)
> >> + return -EMSGSIZE;
> > It is not possible, since user_skb is allocated to accommodate all
> > netlink attributes.
>
> Pravin, common practice is to always check nla_*() return values even if the
> SKB is allocated with "enough space".
>
> Those calculations can have bugs, and these checks are therefore helpful to
> avoid crashes and memory corruption in such cases.
>
OK, in that case this patch needs to proper error handling.
Re: [PATCH] datapath.c: fix missing return value check of nla_nest_start()
From: Pravin Shelar
Date: Tue, 21 Aug 2018 15:38:28 -0700
> On Fri, Aug 17, 2018 at 1:15 AM Jiecheng Wu wrote:
>>
>> Function queue_userspace_packet() defined in net/openvswitch/datapath.c
>> calls nla_nest_start() to allocate memory for struct nlattr which is
>> dereferenced immediately. As nla_nest_start() may return NULL on failure,
>> this code piece may cause NULL pointer dereference bug.
>> ---
>> net/openvswitch/datapath.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index 0f5ce77..ff4457d 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -460,6 +460,8 @@ static int queue_userspace_packet(struct datapath *dp,
>> struct sk_buff *skb,
>>
>> if (upcall_info->egress_tun_info) {
>> nla = nla_nest_start(user_skb,
>> OVS_PACKET_ATTR_EGRESS_TUN_KEY);
>> + if (!nla)
>> + return -EMSGSIZE;
> It is not possible, since user_skb is allocated to accommodate all
> netlink attributes.
Pravin, common practice is to always check nla_*() return values even if the
SKB is allocated with "enough space".
Those calculations can have bugs, and these checks are therefore helpful to
avoid crashes and memory corruption in such cases.
Thank you.
Re: [PATCH] datapath.c: fix missing return value check of nla_nest_start()
On Fri, Aug 17, 2018 at 1:15 AM Jiecheng Wu wrote:
>
> Function queue_userspace_packet() defined in net/openvswitch/datapath.c calls
> nla_nest_start() to allocate memory for struct nlattr which is dereferenced
> immediately. As nla_nest_start() may return NULL on failure, this code piece
> may cause NULL pointer dereference bug.
> ---
> net/openvswitch/datapath.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 0f5ce77..ff4457d 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -460,6 +460,8 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
>
> if (upcall_info->egress_tun_info) {
> nla = nla_nest_start(user_skb,
> OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> + if (!nla)
> + return -EMSGSIZE;
It is not possible, since user_skb is allocated to accommodate all
netlink attributes.
> err = ovs_nla_put_tunnel_info(user_skb,
> upcall_info->egress_tun_info);
> BUG_ON(err);
> @@ -468,6 +470,8 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
>
> if (upcall_info->actions_len) {
> nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_ACTIONS);
> + if (!nla)
> + return -EMSGSIZE;
same as above, the check is not required.
> err = ovs_nla_put_actions(upcall_info->actions,
> upcall_info->actions_len,
> user_skb);
> --
> 2.6.4
>
[PATCH] datapath.c: fix missing return value check of nla_nest_start()
Function queue_userspace_packet() defined in net/openvswitch/datapath.c calls
nla_nest_start() to allocate memory for struct nlattr which is dereferenced
immediately. As nla_nest_start() may return NULL on failure, this code piece
may cause NULL pointer dereference bug.
---
net/openvswitch/datapath.c | 4
1 file changed, 4 insertions(+)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 0f5ce77..ff4457d 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -460,6 +460,8 @@ static int queue_userspace_packet(struct datapath *dp,
struct sk_buff *skb,
if (upcall_info->egress_tun_info) {
nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_EGRESS_TUN_KEY);
+ if (!nla)
+ return -EMSGSIZE;
err = ovs_nla_put_tunnel_info(user_skb,
upcall_info->egress_tun_info);
BUG_ON(err);
@@ -468,6 +470,8 @@ static int queue_userspace_packet(struct datapath *dp,
struct sk_buff *skb,
if (upcall_info->actions_len) {
nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_ACTIONS);
+ if (!nla)
+ return -EMSGSIZE;
err = ovs_nla_put_actions(upcall_info->actions,
upcall_info->actions_len,
user_skb);
--
2.6.4
