On 12/13/23 23:19, Frode Nordahl wrote:
> In the event a schema conversion aborts, the cleanup code in
> ovsdb_convert() prior to this patch will remove the in-memory
> copy of the new database prior to aborting any on-going
> transactions in that database, consequently leading to a use after
> free and potential crash.
>
> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered
> databases.")
> Signed-off-by: Frode Nordahl <frode.nord...@canonical.com>
> ---
> ovsdb/file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 8bd1d4af3..778b4004b 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -388,10 +388,10 @@ ovsdb_convert(const struct ovsdb *src, const struct
> ovsdb_schema *new_schema,
> return NULL;
>
> error:
> - ovsdb_destroy(dst);
> if (txn) {
> ovsdb_txn_abort(txn);
> }
> + ovsdb_destroy(dst);
> *dstp = NULL;
> return error;
> }
Thanks, Frode! Good catch!
Can we have a test case for this issue though?
I don't think we can test this directly, as the chances for the
crash are not 100%, but we can write a test and let asan fail it.
We do run tests under asan in CI, so that should be fine.
The reproducer may look like this:
1. Create a schema with 2 tables with string columns.
2. Create database and add non-numerical string data to both tables.
3. Try to convert the column in the first table to integer.
4. The same for the other table.
Steps 3 and 4 should expect failure, but should ignore the
error message, or at least ignore the table/column names in
the output, because we don't know which table will be first
during conversion. For the same reason we need both steps,
because we need transaction to be populated with the data
from one table before the conversion fails on the second one.
ASAN or valgrind should catch the UAF condition.
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
