| Message ID | 20171204195143.15325-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] odp-util: Use flexible sized buffer to hold Geneve options. | expand |
> On Dec 4, 2017, at 11:51 AM, Ben Pfaff <blp@ovn.org> wrote: > > The 'mask' buffer in parse_odp_action() is supposed to always be big > enough: > /* 'mask' is big enough to hold any key. */ > > Geneve options can be really big and the comment was wrong. In addition, > the user might supply more options than can really fit in any case, so > we might as well just use a stub. > > Found by libfuzzer. > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > Signed-off-by: Ben Pfaff <blp@ovn.org> Acked-by: Justin Pettit <jpettit@ovn.org> --Justin
On Thu, Dec 21, 2017 at 04:30:05PM -0800, Justin Pettit wrote: > > > > On Dec 4, 2017, at 11:51 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > The 'mask' buffer in parse_odp_action() is supposed to always be big > > enough: > > /* 'mask' is big enough to hold any key. */ > > > > Geneve options can be really big and the comment was wrong. In addition, > > the user might supply more options than can really fit in any case, so > > we might as well just use a stub. > > > > Found by libfuzzer. > > > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Acked-by: Justin Pettit <jpettit@ovn.org> Thanks, applied to master and backported as far as 2.4.
> On Thu, Dec 21, 2017 at 04:30:05PM -0800, Justin Pettit wrote: >> >> >> > On Dec 4, 2017, at 11:51 AM, Ben Pfaff <blp at ovn.org> wrote: >> > >> > The 'mask' buffer in parse_odp_action() is supposed to always be big >> > enough: >> > /* 'mask' is big enough to hold any key. */ >> > >> > Geneve options can be really big and the comment was wrong. In addition, >> > the user might supply more options than can really fit in any case, so >> > we might as well just use a stub. >> > >> > Found by libfuzzer. >> > >> > Reported-by: Bhargava Shastry <bshastry at sec.t-labs.tu-berlin.de> >> > Signed-off-by: Ben Pfaff <blp at ovn.org> >> >> Acked-by: Justin Pettit <jpettit at ovn.org> > > Thanks, applied to master and backported as far as 2.4. Hi Ben and Simon. This patch breaks the build on branch-2.4, because there is no such thing as OFPBUF_STUB_INITIALIZER on this branch: lib/odp-util.c:1063:33: error: undefined identifier 'OFPBUF_STUB_INITIALIZER' lib/odp-util.c:1063:56: error: invalid initializer We need to revert it or fix in other way. BTW, seems nobody uses this branch. It was broken for more than a year. Maybe we could stop backporting stuff to it. Best regards, Ilya Maximets.
On Mon, Mar 18, 2019 at 07:13:27PM +0300, Ilya Maximets wrote: > > On Thu, Dec 21, 2017 at 04:30:05PM -0800, Justin Pettit wrote: > >> > >> > >> > On Dec 4, 2017, at 11:51 AM, Ben Pfaff <blp at ovn.org> wrote: > >> > > >> > The 'mask' buffer in parse_odp_action() is supposed to always be big > >> > enough: > >> > /* 'mask' is big enough to hold any key. */ > >> > > >> > Geneve options can be really big and the comment was wrong. In addition, > >> > the user might supply more options than can really fit in any case, so > >> > we might as well just use a stub. > >> > > >> > Found by libfuzzer. > >> > > >> > Reported-by: Bhargava Shastry <bshastry at sec.t-labs.tu-berlin.de> > >> > Signed-off-by: Ben Pfaff <blp at ovn.org> > >> > >> Acked-by: Justin Pettit <jpettit at ovn.org> > > > > Thanks, applied to master and backported as far as 2.4. > > Hi Ben and Simon. > > This patch breaks the build on branch-2.4, because there is no such thing > as OFPBUF_STUB_INITIALIZER on this branch: > > lib/odp-util.c:1063:33: error: undefined identifier 'OFPBUF_STUB_INITIALIZER' > lib/odp-util.c:1063:56: error: invalid initializer > > We need to revert it or fix in other way. > > > BTW, seems nobody uses this branch. It was broken for more than a year. > Maybe we could stop backporting stuff to it. Yes, maybe. Locally, backports of the following seem to help the problem at hand: commit 8812f224fd3d5da88a7a4270f9d9027b21bcd980 Author: Ben Pfaff <blp@nicira.com> Date: Thu Jul 23 16:28:50 2015 -0700 ofpbuf: New macro OFPBUF_STUB_INITIALIZER. To be used in an upcoming commit. Signed-off-by: Ben Pfaff <blp@nicira.com> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> commit 7b71784429099e69870a9d6a3fc7ef7de8ca9cb0 Author: Ben Pfaff <blp@nicira.com> Date: Fri Jul 31 13:14:20 2015 -0700 list: New macro OVS_LIST_POISON for initializing a poisoned list. To be used in an upcoming commit. Signed-off-by: Ben Pfaff <blp@nicira.com> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
On Mon, Mar 18, 2019 at 07:13:27PM +0300, Ilya Maximets wrote: > > On Thu, Dec 21, 2017 at 04:30:05PM -0800, Justin Pettit wrote: > >> > >> > >> > On Dec 4, 2017, at 11:51 AM, Ben Pfaff <blp at ovn.org> wrote: > >> > > >> > The 'mask' buffer in parse_odp_action() is supposed to always be big > >> > enough: > >> > /* 'mask' is big enough to hold any key. */ > >> > > >> > Geneve options can be really big and the comment was wrong. In addition, > >> > the user might supply more options than can really fit in any case, so > >> > we might as well just use a stub. > >> > > >> > Found by libfuzzer. > >> > > >> > Reported-by: Bhargava Shastry <bshastry at sec.t-labs.tu-berlin.de> > >> > Signed-off-by: Ben Pfaff <blp at ovn.org> > >> > >> Acked-by: Justin Pettit <jpettit at ovn.org> > > > > Thanks, applied to master and backported as far as 2.4. > > Hi Ben and Simon. > > This patch breaks the build on branch-2.4, because there is no such thing > as OFPBUF_STUB_INITIALIZER on this branch: > > lib/odp-util.c:1063:33: error: undefined identifier 'OFPBUF_STUB_INITIALIZER' > lib/odp-util.c:1063:56: error: invalid initializer > > We need to revert it or fix in other way. > > > BTW, seems nobody uses this branch. It was broken for more than a year. > Maybe we could stop backporting stuff to it. > > Best regards, Ilya Maximets. I applied the two patches that Simon identified. I will stop backporting beyond branch-2.5. That is a good idea.
diff --git a/lib/odp-util.c b/lib/odp-util.c index a3211118c17c..26e617754dd3 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -1965,20 +1965,19 @@ parse_odp_action(const char *s, const struct simap *port_names, if (!strncmp(s, "set(", 4)) { size_t start_ofs; int retval; - struct nlattr mask[128 / sizeof(struct nlattr)]; - struct ofpbuf maskbuf; + struct nlattr mask[1024 / sizeof(struct nlattr)]; + struct ofpbuf maskbuf = OFPBUF_STUB_INITIALIZER(mask); struct nlattr *nested, *key; size_t size; - /* 'mask' is big enough to hold any key. */ - ofpbuf_use_stack(&maskbuf, mask, sizeof mask); - start_ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_SET); retval = parse_odp_key_mask_attr(s + 4, port_names, actions, &maskbuf); if (retval < 0) { + ofpbuf_uninit(&maskbuf); return retval; } if (s[retval + 4] != ')') { + ofpbuf_uninit(&maskbuf); return -EINVAL; } @@ -2005,6 +2004,7 @@ parse_odp_action(const char *s, const struct simap *port_names, nested->nla_type = OVS_ACTION_ATTR_SET_MASKED; } } + ofpbuf_uninit(&maskbuf); nl_msg_end_nested(actions, start_ofs); return retval + 5;
The 'mask' buffer in parse_odp_action() is supposed to always be big enough: /* 'mask' is big enough to hold any key. */ Geneve options can be really big and the comment was wrong. In addition, the user might supply more options than can really fit in any case, so we might as well just use a stub. Found by libfuzzer. Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> Signed-off-by: Ben Pfaff <blp@ovn.org> --- v1->v2: Use stub because the user can supply more options than fit in any case. lib/odp-util.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)