[PATCH-for-9.0?] backends/cryptodev: Do not abort for invalid session ID

[PATCH-for-9.0?] backends/cryptodev: Do not abort for invalid session ID

[Philippe Mathieu-Daudé]

Instead of aborting when a session ID is invalid,
report an error and return VIRTIO_CRYPTO_INVSESS
("Invalid session id").

Reproduced using:

  $ cat << EOF | qemu-system-i386 -display none \
     -machine q35,accel=qtest -m 512M -nodefaults \
     -object cryptodev-backend-builtin,id=cryptodev0 \
     -device virtio-crypto-pci,id=crypto0,cryptodev=cryptodev0 \
     -qtest stdio
  outl 0xcf8 0x80000804
  outw 0xcfc 0x06
  outl 0xcf8 0x80000820
  outl 0xcfc 0xe0008000
  write 0x10800e 0x1 0x01
  write 0xe0008016 0x1 0x01
  write 0xe0008020 0x4 0x00801000
  write 0xe0008028 0x4 0x00c01000
  write 0xe000801c 0x1 0x01
  write 0x110000 0x1 0x05
  write 0x110001 0x1 0x04
  write 0x108002 0x1 0x11
  write 0x108008 0x1 0x48
  write 0x10800c 0x1 0x01
  write 0x108018 0x1 0x10
  write 0x10801c 0x1 0x02
  write 0x10c002 0x1 0x01
  write 0xe000b005 0x1 0x00
  EOF
  Assertion failed: (session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]),
  function cryptodev_builtin_close_session, file cryptodev-builtin.c, line 430.

Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2274
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/cryptodev-builtin.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 39d0455280..3bbaabe86e 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -22,6 +22,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "sysemu/cryptodev.h"
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_crypto.h"
@@ -427,7 +428,10 @@ static int cryptodev_builtin_close_session(
                       CRYPTODEV_BACKEND_BUILTIN(backend);
     CryptoDevBackendBuiltinSession *session;
 
-    assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]);
+    if (session_id >= MAX_NUM_SESSIONS || !builtin->sessions[session_id]) {
+        error_report("Cannot find a valid session id: %" PRIu64 "", session_id);
+        return -VIRTIO_CRYPTO_INVSESS;
+    }
 
     session = builtin->sessions[session_id];
     if (session->cipher) {
-- 
2.41.0

Re: [PATCH-for-9.0?] backends/cryptodev: Do not abort for invalid session ID

[zhenwei pi]

Hi,

VIRTIO_CRYPTO_INVSESS has a quite clear meaning: invalid session ID when 
executing crypto operations.

Uplayer would get an explicit code once failing to close session, so I 
suggest no error log printing in this function.

On 4/8/24 23:45, Philippe Mathieu-Daudé wrote:
> Instead of aborting when a session ID is invalid,
> report an error and return VIRTIO_CRYPTO_INVSESS
> ("Invalid session id").
> 
> Reproduced using:
> 
>    $ cat << EOF | qemu-system-i386 -display none \
>       -machine q35,accel=qtest -m 512M -nodefaults \
>       -object cryptodev-backend-builtin,id=cryptodev0 \
>       -device virtio-crypto-pci,id=crypto0,cryptodev=cryptodev0 \
>       -qtest stdio
>    outl 0xcf8 0x80000804
>    outw 0xcfc 0x06
>    outl 0xcf8 0x80000820
>    outl 0xcfc 0xe0008000
>    write 0x10800e 0x1 0x01
>    write 0xe0008016 0x1 0x01
>    write 0xe0008020 0x4 0x00801000
>    write 0xe0008028 0x4 0x00c01000
>    write 0xe000801c 0x1 0x01
>    write 0x110000 0x1 0x05
>    write 0x110001 0x1 0x04
>    write 0x108002 0x1 0x11
>    write 0x108008 0x1 0x48
>    write 0x10800c 0x1 0x01
>    write 0x108018 0x1 0x10
>    write 0x10801c 0x1 0x02
>    write 0x10c002 0x1 0x01
>    write 0xe000b005 0x1 0x00
>    EOF
>    Assertion failed: (session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]),
>    function cryptodev_builtin_close_session, file cryptodev-builtin.c, line 430.
> 
> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2274
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/cryptodev-builtin.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
> index 39d0455280..3bbaabe86e 100644
> --- a/backends/cryptodev-builtin.c
> +++ b/backends/cryptodev-builtin.c
> @@ -22,6 +22,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>   #include "sysemu/cryptodev.h"
>   #include "qapi/error.h"
>   #include "standard-headers/linux/virtio_crypto.h"
> @@ -427,7 +428,10 @@ static int cryptodev_builtin_close_session(
>                         CRYPTODEV_BACKEND_BUILTIN(backend);
>       CryptoDevBackendBuiltinSession *session;
>   
> -    assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]);
> +    if (session_id >= MAX_NUM_SESSIONS || !builtin->sessions[session_id]) {
> +        error_report("Cannot find a valid session id: %" PRIu64 "", session_id);
> +        return -VIRTIO_CRYPTO_INVSESS;
> +    }
>   
>       session = builtin->sessions[session_id];
>       if (session->cipher) {

-- 
zhenwei pi