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] [day] [month] [year] [list]

Message-ID: <CAOrHB_B6DYBS+z+0R72w-NRv2_DrPY84Oh9ipTTmg1bqe73JUw@mail.gmail.com>
Date:   Tue, 21 Aug 2018 23:40:34 -0700
From:   Pravin Shelar <pshelar@....org>
To:     "David S. Miller" <davem@...emloft.net>
Cc:     Jason Wood <jasonwood2031@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH] datapath.c: fix missing return value check of nla_nest_start()

On Tue, Aug 21, 2018 at 4:38 PM David Miller <davem@...emloft.net> wrote:
>
> 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.
>
OK, in that case this patch needs to proper error handling.