[ovs-dev] datapath: fix flow actions reallocation
diff mbox series

Message ID 1554936622-4811-1-git-send-email-gvrose8192@gmail.com
State New
Headers show
Series
  • [ovs-dev] datapath: fix flow actions reallocation
Related show

Commit Message

Gregory Rose April 10, 2019, 10:50 p.m. UTC
From: Andrea Righi <andrea.righi@canonical.com>

Upstream commit:
    commit f28cd2af22a0c134e4aa1c64a70f70d815d473fb
    Author: Andrea Righi <andrea.righi@canonical.com>
    Date:   Thu Mar 28 07:36:00 2019 +0100

    openvswitch: fix flow actions reallocation

    The flow action buffer can be resized if it's not big enough to contain
    all the requested flow actions. However, this resize doesn't take into
    account the new requested size, the buffer is only increased by a factor
    of 2x. This might be not enough to contain the new data, causing a
    buffer overflow, for example:

    [   42.044472] =============================================================================
    [   42.045608] BUG kmalloc-96 (Not tainted): Redzone overwritten
    [   42.046415] -----------------------------------------------------------------------------

    [   42.047715] Disabling lock debugging due to kernel taint
    [   42.047716] INFO: 0x8bf2c4a5-0x720c0928. First byte 0x0 instead of 0xcc
    [   42.048677] INFO: Slab 0xbc6d2040 objects=29 used=18 fp=0xdc07dec4 flags=0x2808101
    [   42.049743] INFO: Object 0xd53a3464 @offset=2528 fp=0xccdcdebb

    [   42.050747] Redzone 76f1b237: cc cc cc cc cc cc cc cc                          ........
    [   42.051839] Object d53a3464: 6b 6b 6b 6b 6b 6b 6b 6b 0c 00 00 00 6c 00 00 00  kkkkkkkk....l...
    [   42.053015] Object f49a30cc: 6c 00 0c 00 00 00 00 00 00 00 00 03 78 a3 15 f6  l...........x...
    [   42.054203] Object acfe4220: 20 00 02 00 ff ff ff ff 00 00 00 00 00 00 00 00   ...............
    [   42.055370] Object 21024e91: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    [   42.056541] Object 070e04c3: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    [   42.057797] Object 948a777a: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    [   42.059061] Redzone 8bf2c4a5: 00 00 00 00                                      ....
    [   42.060189] Padding a681b46e: 5a 5a 5a 5a 5a 5a 5a 5a                          ZZZZZZZZ

    Fix by making sure the new buffer is properly resized to contain all the
    requested data.

    BugLink: https://bugs.launchpad.net/bugs/1813244
    Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
    Acked-by: Pravin B Shelar <pshelar@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Cc: Andrea Righi <andrea.righi@canonical.com>
Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 datapath/flow_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

William Tu April 10, 2019, 10:58 p.m. UTC | #1
On Wed, Apr 10, 2019 at 3:51 PM Greg Rose <gvrose8192@gmail.com> wrote:
>
> From: Andrea Righi <andrea.righi@canonical.com>
>
> Upstream commit:
>     commit f28cd2af22a0c134e4aa1c64a70f70d815d473fb
>     Author: Andrea Righi <andrea.righi@canonical.com>
>     Date:   Thu Mar 28 07:36:00 2019 +0100
>
>     openvswitch: fix flow actions reallocation
>
>     The flow action buffer can be resized if it's not big enough to contain
>     all the requested flow actions. However, this resize doesn't take into
>     account the new requested size, the buffer is only increased by a factor
>     of 2x. This might be not enough to contain the new data, causing a
>     buffer overflow, for example:
>
>     [   42.044472] =============================================================================
>     [   42.045608] BUG kmalloc-96 (Not tainted): Redzone overwritten
>     [   42.046415] -----------------------------------------------------------------------------
>
>     [   42.047715] Disabling lock debugging due to kernel taint
>     [   42.047716] INFO: 0x8bf2c4a5-0x720c0928. First byte 0x0 instead of 0xcc
>     [   42.048677] INFO: Slab 0xbc6d2040 objects=29 used=18 fp=0xdc07dec4 flags=0x2808101
>     [   42.049743] INFO: Object 0xd53a3464 @offset=2528 fp=0xccdcdebb
>
>     [   42.050747] Redzone 76f1b237: cc cc cc cc cc cc cc cc                          ........
>     [   42.051839] Object d53a3464: 6b 6b 6b 6b 6b 6b 6b 6b 0c 00 00 00 6c 00 00 00  kkkkkkkk....l...
>     [   42.053015] Object f49a30cc: 6c 00 0c 00 00 00 00 00 00 00 00 03 78 a3 15 f6  l...........x...
>     [   42.054203] Object acfe4220: 20 00 02 00 ff ff ff ff 00 00 00 00 00 00 00 00   ...............
>     [   42.055370] Object 21024e91: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     [   42.056541] Object 070e04c3: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     [   42.057797] Object 948a777a: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     [   42.059061] Redzone 8bf2c4a5: 00 00 00 00                                      ....
>     [   42.060189] Padding a681b46e: 5a 5a 5a 5a 5a 5a 5a 5a                          ZZZZZZZZ
>
>     Fix by making sure the new buffer is properly resized to contain all the
>     requested data.
>
>     BugLink: https://bugs.launchpad.net/bugs/1813244
>     Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>     Acked-by: Pravin B Shelar <pshelar@ovn.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Cc: Andrea Righi <andrea.righi@canonical.com>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>

LGTM.
Acked-by: William Tu <u9012063@gmail.com>

> ---
>  datapath/flow_netlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index e5e469a..13e6e33 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -2311,14 +2311,14 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa,
>
>         struct sw_flow_actions *acts;
>         int new_acts_size;
> -       int req_size = NLA_ALIGN(attr_len);
> +       size_t req_size = NLA_ALIGN(attr_len);
>         int next_offset = offsetof(struct sw_flow_actions, actions) +
>                                         (*sfa)->actions_len;
>
>         if (req_size <= (ksize(*sfa) - next_offset))
>                 goto out;
>
> -       new_acts_size = ksize(*sfa) * 2;
> +       new_acts_size = max(next_offset + req_size, ksize(*sfa) * 2);
>
>         if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
>                 if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
0-day Robot April 10, 2019, 11:50 p.m. UTC | #2
Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Andrea Righi <andrea.righi@canonical.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Greg Rose <gvrose8192@gmail.com>
Lines checked: 77, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Gregory Rose April 11, 2019, 1:07 a.m. UTC | #3
On 4/10/2019 4:50 PM, 0-day Robot wrote:
> Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> ERROR: Author Andrea Righi <andrea.righi@canonical.com> needs to sign off.
> WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Greg Rose <gvrose8192@gmail.com>
> Lines checked: 77, Warnings: 1, Errors: 1
>
>
> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>
> Thanks,
> 0-day Robot

I saw this when I ran checkpatch but still think it's in the correct 
format.  Since I'm the one who does a lot of
these things I suppose I should just go fix it...  Maybe key off the 
'Upstream commit' keywords.

- Greg
Ben Pfaff April 12, 2019, 9:26 p.m. UTC | #4
Thanks, applied to master.  Let me know how far it should be backported,
if at all.
Gregory Rose April 15, 2019, 3:57 p.m. UTC | #5
On 4/12/2019 2:26 PM, Ben Pfaff wrote:
> Thanks, applied to master.  Let me know how far it should be backported,
> if at all.
Thank you Ben.

I'd suggest backporting as far as it goes without a patch reject.

- Greg
Juerg Haefliger April 15, 2019, 6:28 p.m. UTC | #6
On Fri, 12 Apr 2019 14:26:11 -0700
Ben Pfaff <blp@ovn.org> wrote:

> Thanks, applied to master.  Let me know how far it should be backported,
> if at all.

This patch is supposed to have:

Fixes: 74f84a5726c7 ("openvswitch: Copy individual actions.")

So all the way back to v3.11.

...Juerg
Ben Pfaff April 15, 2019, 6:31 p.m. UTC | #7
On Mon, Apr 15, 2019 at 08:57:18AM -0700, Gregory Rose wrote:
> 
> 
> On 4/12/2019 2:26 PM, Ben Pfaff wrote:
> > Thanks, applied to master.  Let me know how far it should be backported,
> > if at all.
> Thank you Ben.
> 
> I'd suggest backporting as far as it goes without a patch reject.

OK, done.
Gregory Rose April 15, 2019, 8:21 p.m. UTC | #8
On 4/15/2019 11:28 AM, Juerg Haefliger wrote:
> On Fri, 12 Apr 2019 14:26:11 -0700
> Ben Pfaff <blp@ovn.org> wrote:
>
>> Thanks, applied to master.  Let me know how far it should be backported,
>> if at all.
> This patch is supposed to have:
>
> Fixes: 74f84a5726c7 ("openvswitch: Copy individual actions.")
>
> So all the way back to v3.11.
>
> ...Juerg

When I backport upstream fixes from the Linux kernel I will key off of a 
"Fixes" tag in the original
commit.  If I do not see a "Fixes" tag there then I will not add one 
unless someone points it out during
the review process.  It seems it's too late now.

- Greg

Patch
diff mbox series

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index e5e469a..13e6e33 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -2311,14 +2311,14 @@  static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa,
 
 	struct sw_flow_actions *acts;
 	int new_acts_size;
-	int req_size = NLA_ALIGN(attr_len);
+	size_t req_size = NLA_ALIGN(attr_len);
 	int next_offset = offsetof(struct sw_flow_actions, actions) +
 					(*sfa)->actions_len;
 
 	if (req_size <= (ksize(*sfa) - next_offset))
 		goto out;
 
-	new_acts_size = ksize(*sfa) * 2;
+	new_acts_size = max(next_offset + req_size, ksize(*sfa) * 2);
 
 	if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
 		if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {