[PATCH] tests/qtest/libqos/ahci.c: Avoid NULL dereference in ahci_exec()

[PATCH] tests/qtest/libqos/ahci.c: Avoid NULL dereference in ahci_exec()

[Peter Maydell]

In ahci_exec() we attempt to permit the caller to pass a NULL pointer
for opts_in (in which case we use a default set of options).  However
although we check for NULL when setting up the opts variable at the
top of the function, we unconditionally dereference opts_in at the
end of the function as part of freeing the opts->buffer.

Switch to checking whether the final buffer is the same as the
buffer we started with, instead of assuming the value we started
with is always opts_in->buffer.

At the moment all the callers pass a non-NULL opts argument, so
we never saw any crashes in practice.

Fixes: Coverity CID 1432302
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/libqos/ahci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index 2946abc15ae..fba3e7a954e 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -637,10 +637,13 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
     AHCICommand *cmd;
     int rc;
     AHCIOpts *opts;
+    uint64_t buffer_in;
 
     opts = g_memdup((opts_in == NULL ? &default_opts : opts_in),
                     sizeof(AHCIOpts));
 
+    buffer_in = opts->buffer;
+
     /* No guest buffer provided, create one. */
     if (opts->size && !opts->buffer) {
         opts->buffer = ahci_alloc(ahci, opts->size);
@@ -686,7 +689,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
         g_assert_cmpint(rc, ==, 0);
     }
     ahci_command_free(cmd);
-    if (opts->buffer != opts_in->buffer) {
+    if (opts->buffer != buffer_in) {
         ahci_free(ahci, opts->buffer);
     }
     g_free(opts);
-- 
2.20.1

Re: [PATCH] tests/qtest/libqos/ahci.c: Avoid NULL dereference in ahci_exec()

[Paolo Bonzini]

On 03/11/20 12:52, Peter Maydell wrote:
> In ahci_exec() we attempt to permit the caller to pass a NULL pointer
> for opts_in (in which case we use a default set of options).  However
> although we check for NULL when setting up the opts variable at the
> top of the function, we unconditionally dereference opts_in at the
> end of the function as part of freeing the opts->buffer.
> 
> Switch to checking whether the final buffer is the same as the
> buffer we started with, instead of assuming the value we started
> with is always opts_in->buffer.
> 
> At the moment all the callers pass a non-NULL opts argument, so
> we never saw any crashes in practice.
> 
> Fixes: Coverity CID 1432302
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/libqos/ahci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
> index 2946abc15ae..fba3e7a954e 100644
> --- a/tests/qtest/libqos/ahci.c
> +++ b/tests/qtest/libqos/ahci.c
> @@ -637,10 +637,13 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
>      AHCICommand *cmd;
>      int rc;
>      AHCIOpts *opts;
> +    uint64_t buffer_in;
>  
>      opts = g_memdup((opts_in == NULL ? &default_opts : opts_in),
>                      sizeof(AHCIOpts));
>  
> +    buffer_in = opts->buffer;
> +
>      /* No guest buffer provided, create one. */
>      if (opts->size && !opts->buffer) {
>          opts->buffer = ahci_alloc(ahci, opts->size);
> @@ -686,7 +689,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
>          g_assert_cmpint(rc, ==, 0);
>      }
>      ahci_command_free(cmd);
> -    if (opts->buffer != opts_in->buffer) {
> +    if (opts->buffer != buffer_in) {
>          ahci_free(ahci, opts->buffer);
>      }
>      g_free(opts);
> 

Queued, thanks.

Paolo