* [PATCH] openvswitch: fix flow actions reallocation
@ 2019-03-27 22:11 Andrea Righi
2019-03-28 3:43 ` Pravin Shelar
0 siblings, 1 reply; 3+ messages in thread
From: Andrea Righi @ 2019-03-27 22:11 UTC (permalink / raw)
To: Pravin B Shelar; +Cc: David S. Miller, netdev, dev, linux-kernel
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>
---
net/openvswitch/flow_netlink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 691da853bef5..e6f789badaa3 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2306,14 +2306,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(req_size, ksize(*sfa) * 2);
if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {
--
2.19.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] openvswitch: fix flow actions reallocation
2019-03-27 22:11 [PATCH] openvswitch: fix flow actions reallocation Andrea Righi
@ 2019-03-28 3:43 ` Pravin Shelar
2019-03-28 5:46 ` Andrea Righi
0 siblings, 1 reply; 3+ messages in thread
From: Pravin Shelar @ 2019-03-28 3:43 UTC (permalink / raw)
To: Andrea Righi
Cc: David S. Miller, Linux Kernel Network Developers, ovs dev, linux-kernel
On Wed, Mar 27, 2019 at 3:11 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> 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>
This must be rare combination of action that trigger this case.
> ---
> net/openvswitch/flow_netlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 691da853bef5..e6f789badaa3 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -2306,14 +2306,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(req_size, ksize(*sfa) * 2);
>
Don't we need to compare current_action_size+req_size and
current_action_size*2 here ?
> if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
> if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] openvswitch: fix flow actions reallocation
2019-03-28 3:43 ` Pravin Shelar
@ 2019-03-28 5:46 ` Andrea Righi
0 siblings, 0 replies; 3+ messages in thread
From: Andrea Righi @ 2019-03-28 5:46 UTC (permalink / raw)
To: Pravin Shelar
Cc: David S. Miller, Linux Kernel Network Developers, ovs dev, linux-kernel
On Wed, Mar 27, 2019 at 08:43:54PM -0700, Pravin Shelar wrote:
> On Wed, Mar 27, 2019 at 3:11 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > 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>
>
> This must be rare combination of action that trigger this case.
It is pretty rare indeed, but the test case reported in the BugLink can
trigger it 100% of the times.
>
> > ---
> > net/openvswitch/flow_netlink.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 691da853bef5..e6f789badaa3 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -2306,14 +2306,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(req_size, ksize(*sfa) * 2);
> >
> Don't we need to compare current_action_size+req_size and
> current_action_size*2 here ?
You are right! We should compare next_offset+req_size and
ksize(*sfa)*2.
Will send a new patch soon, thanks!
-Andrea
>
> > if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
> > if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {
> > --
> > 2.19.1
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-28 5:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-27 22:11 [PATCH] openvswitch: fix flow actions reallocation Andrea Righi
2019-03-28 3:43 ` Pravin Shelar
2019-03-28 5:46 ` Andrea Righi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).