[SRU] [X/B/C/D] [PATCH v2 0/1] openvswitch: fix kernel buffer overflow

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [X/B/C/D] [PATCH v2 0/1] openvswitch: fix kernel buffer overflow

Andrea Righi
Buglink: https://bugs.launchpad.net/bugs/1813244

[Impact]

 * Flow action buffer can be incorrectly resized to contain the newly added
   action entries: the buffer is always resized multiplying the previous size
   by 2, but this might be not enough if the requested size is bigger than
   previous_size*2, causing a buffer overflow

 * The fix correctly resizes the buffer to prevent the buffer overflow
   and potential memory corruptions

 * This bug can be triggered potentially on any architecture, but it is very
   likely to happen on i386 running the following test case

[Test Case]

 * run this openvswitch test case:
   https://launchpadlibrarian.net/416589265/lp1262692

[Fix]

 * Instead of resizing the buffer by a factor of 2, use
   max(current_size * 2, current_size + requested_size)

[Regression Potential]

 * Fix has been tested on the affected platform and verified using slub_debug.
   It is an upstream fix and also a very small patch (one-liner basically), so
   backport changes are minimal.

Changes in v2:
 - fix has been merged upstream (add reference to the proper sha1)

Andrea Righi (1):
 openvswitch: fix flow actions reallocation

 net/openvswitch/flow_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU] [X/B/C/D] [PATCH v2 1/1] openvswitch: fix flow actions reallocation

Andrea Righi
BugLink: https://bugs.launchpad.net/bugs/1813244

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 <[hidden email]>
Acked-by: Pravin B Shelar <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from commit f28cd2af22a0c134e4aa1c64a70f70d815d473fb)
Signed-off-by: Andrea Righi <[hidden email]>
---
 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..4bdf5e3ac208 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(next_offset + req_size, ksize(*sfa) * 2);
 
  if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
  if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {
--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU] [X/B/C/D] [PATCH v2 1/1] openvswitch: fix flow actions reallocation

Juerg Haefliger
On Fri,  5 Apr 2019 09:31:53 +0200
Andrea Righi <[hidden email]> wrote:

> BugLink: https://bugs.launchpad.net/bugs/1813244
>
> 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 <[hidden email]>
> Acked-by: Pravin B Shelar <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (cherry picked from commit f28cd2af22a0c134e4aa1c64a70f70d815d473fb)
> Signed-off-by: Andrea Righi <[hidden email]>
Clean cherry pick from upstream and indeed seems to fix LP: #1736390. Very nice!

Acked-by: Juerg Haefliger <[hidden email]>


> ---
>  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..4bdf5e3ac208 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(next_offset + req_size, ksize(*sfa) * 2);
>  
>   if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
>   if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU] [X/B/C/D] [PATCH v2 1/1] openvswitch: fix flow actions reallocation

Juerg Haefliger
Resending with ACK in the email subject :-/

> On Fri,  5 Apr 2019 09:31:53 +0200
> Andrea Righi <[hidden email]> wrote:
>
> > BugLink: https://bugs.launchpad.net/bugs/1813244
> >
> > 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 <[hidden email]>
> > Acked-by: Pravin B Shelar <[hidden email]>
> > Signed-off-by: David S. Miller <[hidden email]>
> > (cherry picked from commit f28cd2af22a0c134e4aa1c64a70f70d815d473fb)
> > Signed-off-by: Andrea Righi <[hidden email]>  
>
> Clean cherry pick from upstream and indeed seems to fix LP: #1736390. Very nice!
>
> Acked-by: Juerg Haefliger <[hidden email]>
>
>
> > ---
> >  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..4bdf5e3ac208 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(next_offset + req_size, ksize(*sfa) * 2);
> >  
> >   if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
> >   if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {  
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU] [X/B/C/D] [PATCH v2 1/1] openvswitch: fix flow actions reallocation

Colin Ian King-2
In reply to this post by Andrea Righi
On 05/04/2019 08:31, Andrea Righi wrote:

> BugLink: https://bugs.launchpad.net/bugs/1813244
>
> 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 <[hidden email]>
> Acked-by: Pravin B Shelar <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (cherry picked from commit f28cd2af22a0c134e4aa1c64a70f70d815d473fb)
> Signed-off-by: Andrea Righi <[hidden email]>
> ---
>  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..4bdf5e3ac208 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(next_offset + req_size, ksize(*sfa) * 2);
>  
>   if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
>   if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) {
>

Clean cherry pick. Thanks!

Acked-by: Colin Ian King <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED[D]: [SRU] [X/B/C/D] [PATCH v2 0/1] openvswitch: fix kernel buffer overflow

Seth Forshee
In reply to this post by Andrea Righi
On Fri, Apr 05, 2019 at 09:31:52AM +0200, Andrea Righi wrote:

> Buglink: https://bugs.launchpad.net/bugs/1813244
>
> [Impact]
>
>  * Flow action buffer can be incorrectly resized to contain the newly added
>    action entries: the buffer is always resized multiplying the previous size
>    by 2, but this might be not enough if the requested size is bigger than
>    previous_size*2, causing a buffer overflow
>
>  * The fix correctly resizes the buffer to prevent the buffer overflow
>    and potential memory corruptions
>
>  * This bug can be triggered potentially on any architecture, but it is very
>    likely to happen on i386 running the following test case
>
> [Test Case]
>
>  * run this openvswitch test case:
>    https://launchpadlibrarian.net/416589265/lp1262692
>
> [Fix]
>
>  * Instead of resizing the buffer by a factor of 2, use
>    max(current_size * 2, current_size + requested_size)
>
> [Regression Potential]
>
>  * Fix has been tested on the affected platform and verified using slub_debug.
>    It is an upstream fix and also a very small patch (one-liner basically), so
>    backport changes are minimal.
>
> Changes in v2:
>  - fix has been merged upstream (add reference to the proper sha1)

Applied to disco/master-next, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU] [X/B/C/D] [PATCH v2 0/1] openvswitch: fix kernel buffer overflow

Juerg Haefliger
In reply to this post by Andrea Righi
Applied to Xenial/Bionic/Cosmic master-next.

Thanks!
...Juerg


> Buglink: https://bugs.launchpad.net/bugs/1813244
>
> [Impact]
>
>  * Flow action buffer can be incorrectly resized to contain the newly added
>    action entries: the buffer is always resized multiplying the previous size
>    by 2, but this might be not enough if the requested size is bigger than
>    previous_size*2, causing a buffer overflow
>
>  * The fix correctly resizes the buffer to prevent the buffer overflow
>    and potential memory corruptions
>
>  * This bug can be triggered potentially on any architecture, but it is very
>    likely to happen on i386 running the following test case
>
> [Test Case]
>
>  * run this openvswitch test case:
>    https://launchpadlibrarian.net/416589265/lp1262692
>
> [Fix]
>
>  * Instead of resizing the buffer by a factor of 2, use
>    max(current_size * 2, current_size + requested_size)
>
> [Regression Potential]
>
>  * Fix has been tested on the affected platform and verified using slub_debug.
>    It is an upstream fix and also a very small patch (one-liner basically), so
>    backport changes are minimal.
>
> Changes in v2:
>  - fix has been merged upstream (add reference to the proper sha1)
>
> Andrea Righi (1):
>  openvswitch: fix flow actions reallocation
>
>  net/openvswitch/flow_netlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment