Re: [PATCH 126/190] Revert "net: openvswitch: fix a NULL pointer dereference"

[Greg Kroah-Hartman]

On Wed, Apr 21, 2021 at 09:09:56PM -0700, Joe Stringer wrote:
> On Wed, Apr 21, 2021 at 5:01 PM Matteo Croce <mcroce@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, 21 Apr 2021 15:00:01 +0200
> > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > > This reverts commit 6f19893b644a9454d85e593b5e90914e7a72b7dd.
> > >
> > > Commits from @umn.edu addresses have been found to be submitted in
> > > "bad faith" to try to test the kernel community's ability to review
> > > "known malicious" changes.  The result of these submissions can be
> > > found in a paper published at the 42nd IEEE Symposium on Security and
> > > Privacy entitled, "Open Source Insecurity: Stealthily Introducing
> > > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu
> > > (University of Minnesota) and Kangjie Lu (University of Minnesota).
> > >
> > > Because of this, all submissions from this group must be reverted from
> > > the kernel tree and will need to be re-reviewed again to determine if
> > > they actually are a valid fix.  Until that work is complete, remove
> > > this change to ensure that no problems are being introduced into the
> > > codebase.
> > >
> > > Cc: Kangjie Lu <kjlu@xxxxxxx>
> > > Cc: David S. Miller <davem@xxxxxxxxxxxxx>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  net/openvswitch/datapath.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> > > index 9d6ef6cb9b26..99e63f4bbcaf 100644
> > > --- a/net/openvswitch/datapath.c
> > > +++ b/net/openvswitch/datapath.c
> > > @@ -443,10 +443,6 @@ static int queue_userspace_packet(struct
> > > datapath *dp, struct sk_buff *skb,
> > >       upcall = genlmsg_put(user_skb, 0, 0, &dp_packet_genl_family,
> > >                            0, upcall_info->cmd);
> > > -     if (!upcall) {
> > > -             err = -EINVAL;
> > > -             goto out;
> > > -     }
> > >       upcall->dp_ifindex = dp_ifindex;
> > >
> > >       err = ovs_nla_put_key(key, key, OVS_PACKET_ATTR_KEY, false,
> > > user_skb);
> >
> > This patch seems good to me, but given the situation I'd like another
> > pair of eyes on it, at least.
> 
> The revert LGTM.
> 
> A few lines above:
> 
>         len = upcall_msg_size(upcall_info, hlen - cutlen,
>                               OVS_CB(skb)->acts_origlen);
>         user_skb = genlmsg_new(len, GFP_ATOMIC);
>         if (!user_skb) {
>                 err = -ENOMEM;
>                 goto out;
>         }
> 
> upcall_msg_size() calculates the expected size of the buffer,
> including at the very least a nlmsg-aligned sizeof(struct ovs_header),
> plus other constants and also potential (likely) variable lengths
> based on the current flow context.
> 
> genlmsg_new() adds the (nlmsg-aligned) nlmsg header length to the
> calculated length when allocating the buffer, and if the memory
> allocation fails here then the error is already returned.
> 
> I don't then see a way for genlmsg_put() to fail per the hunk in the
> commit here given that its buffer reservation is calculated based on:
> 
>         nlh = nlmsg_put(skb, portid, seq, family->id, GENL_HDRLEN +
>                         family->hdrsize, flags);
> 
> Where family->hdrsize would be sizeof(struct ovs_header) since
> dp_packet_genl_family is the family passed into the genlmsg_put()
> call:
> 
> static struct genl_family dp_packet_genl_family __ro_after_init = {
>         .hdrsize = sizeof(struct ovs_header),
> 
> Even if there were some allocation bug here to be fixed (due to
> miscalculating the buffer size in the first place), I don't see how
> the extra error path in the included patch could catch such an error.
> The original patch doesn't seem necessarily problematic, but it
> doesn't seem like it adds anything of value either (or at least,
> nothing a comment couldn't clearly explain).
> 
> Cheers,
> Joe

Many thanks for the review, now dropping this revert from my tree.

greg k-h