| Message ID | 20170520234107.2711-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, 2017-05-20 at 16:41 -0700, Ben Pfaff wrote: > msg->size isn't the relevant measurement here because we're only supposed > to read 'len' bytes. Reading more than that causes 'len' to underflow to a > large number at the end of the loop. > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > lib/ofp-util.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index bdf89b6c3017..f05ca398c13e 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -2610,7 +2610,7 @@ ofputil_pull_queue_get_config_reply10(struct ofpbuf *msg, > > hdr = ofpbuf_at_assert(msg, 0, sizeof *hdr); > prop_len = ntohs(hdr->len); > - if (prop_len < sizeof *hdr || prop_len > msg->size || prop_len % 8) { > + if (prop_len < sizeof *hdr || prop_len > len || prop_len % 8) { > return OFPERR_OFPBRC_BAD_LEN; > } > Yikes. Good catch. Acked-by: Greg Rose <gvrose8192@gmail.com>
On Sat, May 20, 2017 at 08:59:39PM -0700, Greg Rose wrote: > On Sat, 2017-05-20 at 16:41 -0700, Ben Pfaff wrote: > > msg->size isn't the relevant measurement here because we're only supposed > > to read 'len' bytes. Reading more than that causes 'len' to underflow to a > > large number at the end of the loop. > > > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > lib/ofp-util.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > > index bdf89b6c3017..f05ca398c13e 100644 > > --- a/lib/ofp-util.c > > +++ b/lib/ofp-util.c > > @@ -2610,7 +2610,7 @@ ofputil_pull_queue_get_config_reply10(struct ofpbuf *msg, > > > > hdr = ofpbuf_at_assert(msg, 0, sizeof *hdr); > > prop_len = ntohs(hdr->len); > > - if (prop_len < sizeof *hdr || prop_len > msg->size || prop_len % 8) { > > + if (prop_len < sizeof *hdr || prop_len > len || prop_len % 8) { > > return OFPERR_OFPBRC_BAD_LEN; > > } > > > > Yikes. Good catch. > > Acked-by: Greg Rose <gvrose8192@gmail.com> Thanks! I applied this to master, branch-2.7, branch-2.6, and branch-2.5.
diff --git a/lib/ofp-util.c b/lib/ofp-util.c index bdf89b6c3017..f05ca398c13e 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -2610,7 +2610,7 @@ ofputil_pull_queue_get_config_reply10(struct ofpbuf *msg, hdr = ofpbuf_at_assert(msg, 0, sizeof *hdr); prop_len = ntohs(hdr->len); - if (prop_len < sizeof *hdr || prop_len > msg->size || prop_len % 8) { + if (prop_len < sizeof *hdr || prop_len > len || prop_len % 8) { return OFPERR_OFPBRC_BAD_LEN; }
msg->size isn't the relevant measurement here because we're only supposed to read 'len' bytes. Reading more than that causes 'len' to underflow to a large number at the end of the loop. Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/ofp-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)