Messages in this thread | | | Date | Tue, 27 Apr 2021 18:48:27 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH 126/190] Revert "net: openvswitch: fix a NULL pointer dereference" |
| |
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@linux.microsoft.com> wrote: > > > > On Wed, 21 Apr 2021 15:00:01 +0200 > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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@umn.edu> > > > Cc: David S. Miller <davem@davemloft.net> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > --- > > > 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
| |