[ovs-dev] [PATCH] ofp-ed-props: Fix hang for crafted OpenFlow encap/decap properties.
Ben Pfaff
blp at ovn.org
Thu Aug 16 20:10:12 UTC 2018
On Thu, Aug 16, 2018 at 10:37:15AM -0700, Darrell Ball wrote:
> On Thu, Aug 16, 2018 at 8:51 AM, Ben Pfaff <blp at ovn.org> wrote:
>
> > On Wed, Aug 15, 2018 at 04:55:38PM -0700, Darrell Ball wrote:
> > > On Wed, Aug 15, 2018 at 3:03 PM, Ben Pfaff <blp at ovn.org> wrote:
> > >
> > > > decode_ed_prop() accepted encap/decap properties with a reported
> > length of
> > > > 0, without consuming any data from the property list, which yielded an
> > > > infinite loop.
> > > >
> > > > Reported-at: https://bugs.chromium.org/p/os
> > s-fuzz/issues/detail?id=9918
> > > > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > > > ---
> > > > lib/ofp-ed-props.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c
> > > > index 901da2f0dd1b..28382e01235c 100644
> > > > --- a/lib/ofp-ed-props.c
> > > > +++ b/lib/ofp-ed-props.c
> > > > @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header
> > > > **ofp_prop,
> > >
> > > size_t len = (*ofp_prop)->len;
> > > > size_t pad_len = ROUND_UP(len, 8);
> > > >
> > > > - if (pad_len > *remaining) {
> > > > + if (len < sizeof **ofp_prop || pad_len > *remaining) {
> > > >
> > >
> > > Is *remaining > pad_len valid ?
> > > If it is, which is not intuitive, maybe a comment will help ?
> >
> > Can you help me understand why it would not be valid?
> >
>
>
> I noted that 'len' here is for a single property vs remaining for all
> properties; I
> had somehow thought that the caller, decode_NXAST_RAW_ENCAP(),
> would verify that the remaining length is sufficient before calling
> decode_ed_prop()
> since it is already verifying the overall length - remaining, 'remaining' >
> 0, meaning it handles the list
> of properties.
>
> Below explains better what I had expected, but I still think the existing
> code and your incremental is better, hence
>
> Acked-by: Darrell Ball<dlu998 at gmail.com>
Thanks. I applied my original fix to master.
The following would make this code closer to the style we use
elsewhere. I haven't tested it.
diff --git a/include/openvswitch/ofp-ed-props.h b/include/openvswitch/ofp-ed-props.h
index 306c6fe7302a..6a85fc0f404d 100644
--- a/include/openvswitch/ofp-ed-props.h
+++ b/include/openvswitch/ofp-ed-props.h
@@ -96,8 +96,7 @@ struct ofpact_ed_prop_nsh_tlv {
/* tlv_len octets of metadata value, padded to a multiple of 8 bytes. */
uint8_t data[0];
};
-enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
- struct ofpbuf *out, size_t *remaining);
+enum ofperr decode_ed_prop(struct ofpbuf *in, struct ofpbuf *out);
enum ofperr encode_ed_prop(const struct ofpact_ed_prop **prop,
struct ofpbuf *out);
bool parse_ed_prop_class(const char *str, uint16_t *prop_class);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 93e212f07196..a14fb03c6991 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4320,13 +4320,7 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae,
enum ofp_version ofp_version OVS_UNUSED,
struct ofpbuf *out)
{
- struct ofpact_encap *encap;
- const struct ofp_ed_prop_header *ofp_prop;
- size_t props_len;
- uint16_t n_props = 0;
- int err;
-
- encap = ofpact_put_ENCAP(out);
+ struct ofpact_encap *encap = ofpact_put_ENCAP(out);
encap->ofpact.raw = NXAST_RAW_ENCAP;
switch (ntohl(nae->new_pkt_type)) {
case PT_ETH:
@@ -4339,11 +4333,11 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae,
encap->new_pkt_type = nae->new_pkt_type;
encap->hdr_size = ntohs(nae->hdr_size);
- ofp_prop = nae->props;
- props_len = ntohs(nae->len) - offsetof(struct nx_action_encap, props);
- n_props = 0;
- while (props_len > 0) {
- err = decode_ed_prop(&ofp_prop, out, &props_len);
+ struct ofpbuf properties = ofpbuf_const_initializer(
+ nae->props, ntohs(nae->len) - offsetof(struct nx_action_encap, props));
+ int n_props = 0;
+ while (properties.size) {
+ int err = decode_ed_prop(&properties, out);
if (err) {
return err;
}
diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c
index 28382e01235c..3d1e74ff2358 100644
--- a/lib/ofp-ed-props.c
+++ b/lib/ofp-ed-props.c
@@ -26,30 +26,34 @@
enum ofperr
-decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
- struct ofpbuf *out OVS_UNUSED,
- size_t *remaining)
+decode_ed_prop(struct ofpbuf *in, struct ofpbuf *out)
{
- uint16_t prop_class = ntohs((*ofp_prop)->prop_class);
- uint8_t prop_type = (*ofp_prop)->type;
- size_t len = (*ofp_prop)->len;
+ const struct ofp_ed_prop_header *ofp_prop
+ = ofpbuf_at(in, 0, sizeof *ofp_prop);
+ if (!ofp_prop) {
+ return OFPERR_OFPBAC_BAD_LEN;
+ }
+ size_t len = ofp_prop->len;
size_t pad_len = ROUND_UP(len, 8);
-
- if (len < sizeof **ofp_prop || pad_len > *remaining) {
+ if (len < sizeof *ofp_prop || pad_len > in->size) {
return OFPERR_OFPBAC_BAD_LEN;
}
+ ofpbuf_pull(in, pad_len);
+
+ uint16_t prop_class = ntohs(ofp_prop->prop_class);
+ uint8_t prop_type = ofp_prop->type;
switch (prop_class) {
case OFPPPC_NSH: {
switch (prop_type) {
case OFPPPT_PROP_NSH_MDTYPE: {
struct ofp_ed_prop_nsh_md_type *opnmt =
- ALIGNED_CAST(struct ofp_ed_prop_nsh_md_type *, *ofp_prop);
- if (len > sizeof(*opnmt) || len > *remaining) {
+ ALIGNED_CAST(struct ofp_ed_prop_nsh_md_type *, ofp_prop);
+ if (len != sizeof *opnmt) {
return OFPERR_NXBAC_BAD_ED_PROP;
}
struct ofpact_ed_prop_nsh_md_type *pnmt =
- ofpbuf_put_uninit(out, sizeof(*pnmt));
+ ofpbuf_put_uninit(out, sizeof *pnmt);
pnmt->header.prop_class = prop_class;
pnmt->header.type = prop_type;
pnmt->header.len = len;
@@ -58,13 +62,13 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
}
case OFPPPT_PROP_NSH_TLV: {
struct ofp_ed_prop_nsh_tlv *opnt =
- ALIGNED_CAST(struct ofp_ed_prop_nsh_tlv *, *ofp_prop);
+ ALIGNED_CAST(struct ofp_ed_prop_nsh_tlv *, ofp_prop);
size_t tlv_pad_len = ROUND_UP(opnt->tlv_len, 8);
- if (len != sizeof(*opnt) + tlv_pad_len || len > *remaining) {
+ if (len != sizeof *opnt + tlv_pad_len) {
return OFPERR_NXBAC_BAD_ED_PROP;
}
struct ofpact_ed_prop_nsh_tlv *pnt =
- ofpbuf_put_uninit(out, sizeof(*pnt));
+ ofpbuf_put_uninit(out, sizeof *pnt);
pnt->header.prop_class = prop_class;
pnt->header.type = prop_type;
pnt->header.len = len;
@@ -83,9 +87,6 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
return OFPERR_NXBAC_UNKNOWN_ED_PROP;
}
- *remaining -= pad_len;
- *ofp_prop = ALIGNED_CAST(const struct ofp_ed_prop_header *,
- ((char *)(*ofp_prop) + pad_len));
return 0;
}
More information about the dev
mailing list