| Message ID | 1456993371-9902-3-git-send-email-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 3 March 2016 at 00:22, Joe Stringer <joe@ovn.org> wrote: > When decoding a variable-length action, if the length of the action > exceeds the length storable in a uint16_t then something has gone > terribly wrong. Assert that this is not the case. > > Signed-off-by: Joe Stringer <joe@ovn.org> With Jarno's ack from patch #4, I pushed this to master and branches 2.1-2.5.
On Thu, Mar 03, 2016 at 09:22:50PM +1300, Joe Stringer wrote: > When decoding a variable-length action, if the length of the action > exceeds the length storable in a uint16_t then something has gone > terribly wrong. Assert that this is not the case. > > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > lib/ofp-actions.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index fe1424f137a1..905469b6bbf9 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -7401,8 +7401,12 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len) > void > ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact) > { > + ptrdiff_t len; > + > ovs_assert(ofpact == ofpacts->header); > - ofpact->len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; > + len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; > + ovs_assert(len <= UINT16_MAX); Probably worth adding "len > 0 &&" to that condition, as long as we're at it. Acked-by: Ben Pfaff <blp@ovn.org>
On 5 March 2016 at 13:25, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Mar 03, 2016 at 09:22:50PM +1300, Joe Stringer wrote: >> When decoding a variable-length action, if the length of the action >> exceeds the length storable in a uint16_t then something has gone >> terribly wrong. Assert that this is not the case. >> >> Signed-off-by: Joe Stringer <joe@ovn.org> >> --- >> lib/ofp-actions.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c >> index fe1424f137a1..905469b6bbf9 100644 >> --- a/lib/ofp-actions.c >> +++ b/lib/ofp-actions.c >> @@ -7401,8 +7401,12 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len) >> void >> ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact) >> { >> + ptrdiff_t len; >> + >> ovs_assert(ofpact == ofpacts->header); >> - ofpact->len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; >> + len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; >> + ovs_assert(len <= UINT16_MAX); > > Probably worth adding "len > 0 &&" to that condition, as long as we're > at it. > > Acked-by: Ben Pfaff <blp@ovn.org> Thanks for the review, I'll send a followup patch. (This was already applied to branches 2.1+)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index fe1424f137a1..905469b6bbf9 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -7401,8 +7401,12 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len) void ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact) { + ptrdiff_t len; + ovs_assert(ofpact == ofpacts->header); - ofpact->len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; + len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; + ovs_assert(len <= UINT16_MAX); + ofpact->len = len; ofpbuf_padto(ofpacts, OFPACT_ALIGN(ofpacts->size)); }
When decoding a variable-length action, if the length of the action exceeds the length storable in a uint16_t then something has gone terribly wrong. Assert that this is not the case. Signed-off-by: Joe Stringer <joe@ovn.org> --- lib/ofp-actions.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)