netdev - Re: [PATCH] datapath.c: fix missing return value check of nla_nest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180821.163806.1699637902725064185.davem@davemloft.net>
Date:   Tue, 21 Aug 2018 16:38:06 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     pshelar@....org
Cc:     jasonwood2031@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] datapath.c: fix missing return value check of
 nla_nest_start()

From: Pravin Shelar <pshelar@....org>
Date: Tue, 21 Aug 2018 15:38:28 -0700

> On Fri, Aug 17, 2018 at 1:15 AM Jiecheng Wu <jasonwood2031@...il.com> 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.