Message ID | 1588440525-71022-4-git-send-email-u9012063@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | ovsdb-idl: Fix memory leak and NULL deref | expand |
Thanks William, this patch looks good to me. Maybe code will be a little neater with the fixes below: @@ -1017,6 +1017,9 @@ static void free_data(enum ovsdb_atomic_type type, union ovsdb_atom *atoms, size_t n_atoms) { + if (!atoms) { + return; + } if (ovsdb_atom_needs_destruction(type)) { Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> Yifeng On Sat, May 2, 2020 at 10:29 AM William Tu <u9012063@gmail.com> wrote: > When 'datum.values' or 'datum.keys' is NULL, some code path calling > into ovsdb_idl_txn_write__ triggers NULL deref. An example is below: > > ovsrec_open_vswitch_set_cur_cfg(const struct ovsrec_open_vswitch > { > struct ovsdb_datum datum; > union ovsdb_atom key; > > datum.n = 1; > datum.keys = &key; > > key.integer = cur_cfg; > // 1. assign_zero: Assigning: datum.values = NULL. > datum.values = NULL; > // CID 1421356 (#1 of 1): Explicit null dereferenced (FORWARD_NULL) > // 2. var_deref_model: Passing &datum to ovsdb_idl_txn_write_clone,\ > // which dereferences null datum.values. > ovsdb_idl_txn_write_clone(&row->header_, &ovsrec_open_vswitch_col > } > > And with the following calls: > ovsdb_idl_txn_write_clone > ovsdb_idl_txn_write__ > 6. deref_parm_in_call: Function ovsdb_datum_destroy dereferences > datum->values > ovsdb_datum_destroy > > And another possible NULL deref is at ovsdb_datum_equals(). Fix the > two by adding additional checks. > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/ovsdb-data.c | 8 ++++++-- > lib/ovsdb-idl.c | 3 ++- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c > index 4828624f658d..9ce3cdeca28a 100644 > --- a/lib/ovsdb-data.c > +++ b/lib/ovsdb-data.c > @@ -1033,8 +1033,12 @@ free_data(enum ovsdb_atomic_type type, > void > ovsdb_datum_destroy(struct ovsdb_datum *datum, const struct ovsdb_type > *type) > { > - free_data(type->key.type, datum->keys, datum->n); > - free_data(type->value.type, datum->values, datum->n); > + if (datum->keys) { > + free_data(type->key.type, datum->keys, datum->n); > + } > + if (datum->values) { > + free_data(type->value.type, datum->values, datum->n); > + } > } > > /* Swaps the contents of 'a' and 'b', which need not have the same type. > */ > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 1535ad7b5197..6614ea1617ef 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -4449,7 +4449,8 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row > *row_, > * transaction only does writes of existing values, without making > any real > * changes, we will drop the whole transaction later in > * ovsdb_idl_txn_commit().) */ > - if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), > + if (datum->keys && datum->values && > + write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), > datum, &column->type)) { > goto discard_datum; > } > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Sat, May 9, 2020 at 11:02 AM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > Thanks William, this patch looks good to me. > Maybe code will be a little neater with the fixes below: > > @@ -1017,6 +1017,9 @@ static void > free_data(enum ovsdb_atomic_type type, > union ovsdb_atom *atoms, size_t n_atoms) > { > + if (!atoms) { > + return; > + } > if (ovsdb_atom_needs_destruction(type)) { > > > > Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> > Thank you. I applied the series to master. William
diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c index 4828624f658d..9ce3cdeca28a 100644 --- a/lib/ovsdb-data.c +++ b/lib/ovsdb-data.c @@ -1033,8 +1033,12 @@ free_data(enum ovsdb_atomic_type type, void ovsdb_datum_destroy(struct ovsdb_datum *datum, const struct ovsdb_type *type) { - free_data(type->key.type, datum->keys, datum->n); - free_data(type->value.type, datum->values, datum->n); + if (datum->keys) { + free_data(type->key.type, datum->keys, datum->n); + } + if (datum->values) { + free_data(type->value.type, datum->values, datum->n); + } } /* Swaps the contents of 'a' and 'b', which need not have the same type. */ diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 1535ad7b5197..6614ea1617ef 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -4449,7 +4449,8 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_, * transaction only does writes of existing values, without making any real * changes, we will drop the whole transaction later in * ovsdb_idl_txn_commit().) */ - if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), + if (datum->keys && datum->values && + write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column), datum, &column->type)) { goto discard_datum; }
When 'datum.values' or 'datum.keys' is NULL, some code path calling into ovsdb_idl_txn_write__ triggers NULL deref. An example is below: ovsrec_open_vswitch_set_cur_cfg(const struct ovsrec_open_vswitch { struct ovsdb_datum datum; union ovsdb_atom key; datum.n = 1; datum.keys = &key; key.integer = cur_cfg; // 1. assign_zero: Assigning: datum.values = NULL. datum.values = NULL; // CID 1421356 (#1 of 1): Explicit null dereferenced (FORWARD_NULL) // 2. var_deref_model: Passing &datum to ovsdb_idl_txn_write_clone,\ // which dereferences null datum.values. ovsdb_idl_txn_write_clone(&row->header_, &ovsrec_open_vswitch_col } And with the following calls: ovsdb_idl_txn_write_clone ovsdb_idl_txn_write__ 6. deref_parm_in_call: Function ovsdb_datum_destroy dereferences datum->values ovsdb_datum_destroy And another possible NULL deref is at ovsdb_datum_equals(). Fix the two by adding additional checks. Signed-off-by: William Tu <u9012063@gmail.com> --- lib/ovsdb-data.c | 8 ++++++-- lib/ovsdb-idl.c | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-)