[ovs-dev] [PATCH 3/3] ovsdb-idl: Fix NULL deref reported by Coverity.

[Yifeng Sun]

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 at gmail.com>

Yifeng

On Sat, May 2, 2020 at 10:29 AM William Tu <u9012063 at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>