Message ID | 1458651757-102018-1-git-send-email-jpettit@ovn.org |
---|---|
State | Accepted |
Headers | show |
On Tue, Mar 22, 2016 at 06:02:35AM -0700, Justin Pettit wrote: > From: Ben Pfaff <blp@ovn.org> > > A crafted MPLS packet yields a zero 'count' in this excerpt from > miniflow_extract(): > > count = parse_mpls(&data, &size); > miniflow_push_words_32(mf, mpls_lse, mpls, count); > > In turn, miniflow_push_words_32() updated mf.map as follows: > > MF.map |= ((UINT64_MAX >> (64 - DIV_ROUND_UP(N_WORDS, 2))) << ofs64); > > which expanded to: > > mf.map |= (UINT64_MAX >> 64) << ofs64; > > Unforunately, C renders shifting a 64-bit constant by 64 bits undefined. > On common x86 platforms, 'n << 64' is equal to 'n', so this behaves as: > > mf.map |= UINT64_MAX << ofs64; > > In this particular case, ofs64 is 15, so this sets the most-significant 48 > bits of mf.map (a 63-bit bit-field) to 1. Only the least-significant 28 > bits of mf.map should ever be set to 1, so this sets 35 bits to 1 that > should never be. Because of the structure of the data structure that > mf.map is embedded within, this makes it possible later to overwrite 8*35 > == 280 bytes of data in the stack. However, there is no obvious way to > control the data used in the overwrite--it is memcpy'd from one place to > another but the source data does not come from the network. In the bug > reporter's testing, this overwrite caused a userspace crash if debug > logging was enabled, but not otherwise. > > This commit fixes the problem by avoiding the out-of-range shift. > > Vulnerability: CVE-2016-2074 > Reported-by: Kashyap Thimmaraju <kashyap.thimmaraju@sec.t-labs.tu-berlin.de> > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > Signed-off-by: Ben Pfaff <blp@ovn.org> > Acked-by: Jesse Gross <jesse@kernel.org> Jesse acked this one, privately. It's my patch so I can't ack it ;-)
> On Mar 28, 2016, at 5:26 PM, Ben Pfaff <blp@ovn.org> wrote: > >> Vulnerability: CVE-2016-2074 >> Reported-by: Kashyap Thimmaraju <kashyap.thimmaraju@sec.t-labs.tu-berlin.de> >> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> >> Signed-off-by: Ben Pfaff <blp@ovn.org> >> Acked-by: Jesse Gross <jesse@kernel.org> > > Jesse acked this one, privately. It's my patch so I can't ack it ;-) Yep. I just wanted to get it out on the mailing list so there wouldn't be any gaps. Thanks for the reviews. --Justin
diff --git a/lib/flow.c b/lib/flow.c index 5df23a9..03c175a 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -197,7 +197,7 @@ BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() will have runtime " /* Data at 'valuep' may be unaligned. */ #define miniflow_push_words_(MF, OFS, VALUEP, N_WORDS) \ -{ \ +if (N_WORDS) { \ int ofs64 = (OFS) / 8; \ \ MINIFLOW_ASSERT(MF.data + (N_WORDS) <= MF.end && (OFS) % 8 == 0 \ @@ -210,7 +210,7 @@ BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() will have runtime " /* Push 32-bit words padded to 64-bits. */ #define miniflow_push_words_32_(MF, OFS, VALUEP, N_WORDS) \ -{ \ +if (N_WORDS) { \ int ofs64 = (OFS) / 8; \ \ MINIFLOW_ASSERT(MF.data + DIV_ROUND_UP(N_WORDS, 2) <= MF.end \