| Message ID | 20231213221928.3224497-1-frode.nordahl@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] ovsdb: Fix use after free on schema conversion error. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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.nordahl@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.
On Fri, Dec 15, 2023 at 7:24 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > 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.nordahl@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? Thanks alot for taking the time to review, much appreciated! A test case for this sounds like a good idea, I'll have a look.
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; }
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.nordahl@canonical.com> --- ovsdb/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)