[PATCH] hw/nvme: Add options to override hardcoded values

[PATCH] hw/nvme: Add options to override hardcoded values

[Mauricio Sandt]

This small patch is the result of some recent malware research I did
in a QEMU VM. The malware used multiple ways of querying info from
the VM disk and I needed a clean way to change those values from the
hypervisor.

I believe this functionality could be useful to more people from multiple
fields, sometimes you just want to change some default values and having
them hardcoded in the sourcecode makes that much harder.

This patch adds three config parameters to the nvme device, all of them
are optional to not break anything. If any of them are not specified,
the previous (before this patch) default is used.

-model - This takes a string and sets it as the devices model name.
If you don't specify this parameter, the default is "QEMU NVMe Ctrl".

-firmware - The firmware version string, max 8 ascii characters.
The default is whatever `QEMU_VERSION` evaluates to.

-nqn_override - Allows to set a custom nqn on the nvme device.
Only used if there is no subsystem. This string should be in the same
format as the default "nqn.2019-08.org.qemu:...", but there is no
validation for that. Its up to the user to provide a valid string.

Signed-off-by: Mauricio Sandt <mauricio@mailbox.org>
---
 hw/nvme/ctrl.c | 16 +++++++++++++---
 hw/nvme/nvme.h |  3 +++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 1e6e0fcad9..0e67217a63 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6697,8 +6697,13 @@ static void nvme_init_subnqn(NvmeCtrl *n)
     NvmeIdCtrl *id = &n->id_ctrl;
 
     if (!subsys) {
-        snprintf((char *)id->subnqn, sizeof(id->subnqn),
+        if (n->params.nqn_override) {
+            snprintf((char *)id->subnqn, sizeof(id->subnqn),
+                 "%s", n->params.nqn_override);
+        } else {
+            snprintf((char *)id->subnqn, sizeof(id->subnqn),
                  "nqn.2019-08.org.qemu:%s", n->params.serial);
+        }
     } else {
         pstrcpy((char *)id->subnqn, sizeof(id->subnqn), (char*)subsys->subnqn);
     }
@@ -6712,8 +6717,10 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 
     id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
     id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
-    strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' ');
-    strpadcpy((char *)id->fr, sizeof(id->fr), QEMU_VERSION, ' ');
+    strpadcpy((char *)id->mn, sizeof(id->mn),
+            n->params.model ? n->params.model : "QEMU NVMe Ctrl", ' ');
+    strpadcpy((char *)id->fr, sizeof(id->fr),
+            n->params.firmware ? n->params.firmware : QEMU_VERSION, ' ');
     strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' ');
 
     id->cntlid = cpu_to_le16(n->cntlid);
@@ -6913,6 +6920,9 @@ static Property nvme_props[] = {
     DEFINE_PROP_LINK("subsys", NvmeCtrl, subsys, TYPE_NVME_SUBSYS,
                      NvmeSubsystem *),
     DEFINE_PROP_STRING("serial", NvmeCtrl, params.serial),
+    DEFINE_PROP_STRING("model", NvmeCtrl, params.model),
+    DEFINE_PROP_STRING("nqn_override", NvmeCtrl, params.nqn_override),
+    DEFINE_PROP_STRING("firmware", NvmeCtrl, params.firmware),
     DEFINE_PROP_UINT32("cmb_size_mb", NvmeCtrl, params.cmb_size_mb, 0),
     DEFINE_PROP_UINT32("num_queues", NvmeCtrl, params.num_queues, 0),
     DEFINE_PROP_UINT32("max_ioqpairs", NvmeCtrl, params.max_ioqpairs, 64),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index e41771604f..45bcf3e02e 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -394,6 +394,9 @@ typedef struct NvmeCQueue {
 
 typedef struct NvmeParams {
     char     *serial;
+    char     *model;
+    char     *firmware;
+    char     *nqn_override;
     uint32_t num_queues; /* deprecated since 5.1 */
     uint32_t max_ioqpairs;
     uint16_t msix_qsize;
-- 
2.25.1

Ping: [PATCH] hw/nvme: Add options to override hardcoded values

[Mauricio Sandt]

[-- Attachment #1: Type: text/plain, Size: 4182 bytes --]

https://patchew.org/QEMU/20220611223509.32280-1-mauricio@mailbox.org/
https://lore.kernel.org/qemu-devel/20220611223509.32280-1-mauricio@mailbox.org/

On 12/06/2022 00:35, Mauricio Sandt wrote:
> This small patch is the result of some recent malware research I did
> in a QEMU VM. The malware used multiple ways of querying info from
> the VM disk and I needed a clean way to change those values from the
> hypervisor.
>
> I believe this functionality could be useful to more people from multiple
> fields, sometimes you just want to change some default values and having
> them hardcoded in the sourcecode makes that much harder.
>
> This patch adds three config parameters to the nvme device, all of them
> are optional to not break anything. If any of them are not specified,
> the previous (before this patch) default is used.
>
> -model - This takes a string and sets it as the devices model name.
> If you don't specify this parameter, the default is "QEMU NVMe Ctrl".
>
> -firmware - The firmware version string, max 8 ascii characters.
> The default is whatever `QEMU_VERSION` evaluates to.
>
> -nqn_override - Allows to set a custom nqn on the nvme device.
> Only used if there is no subsystem. This string should be in the same
> format as the default "nqn.2019-08.org.qemu:...", but there is no
> validation for that. Its up to the user to provide a valid string.
>
> Signed-off-by: Mauricio Sandt<mauricio@mailbox.org>
> ---
>   hw/nvme/ctrl.c | 16 +++++++++++++---
>   hw/nvme/nvme.h |  3 +++
>   2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 1e6e0fcad9..0e67217a63 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6697,8 +6697,13 @@ static void nvme_init_subnqn(NvmeCtrl *n)
>       NvmeIdCtrl *id = &n->id_ctrl;
>   
>       if (!subsys) {
> -        snprintf((char *)id->subnqn, sizeof(id->subnqn),
> +        if (n->params.nqn_override) {
> +            snprintf((char *)id->subnqn, sizeof(id->subnqn),
> +                 "%s", n->params.nqn_override);
> +        } else {
> +            snprintf((char *)id->subnqn, sizeof(id->subnqn),
>                    "nqn.2019-08.org.qemu:%s", n->params.serial);
> +        }
>       } else {
>           pstrcpy((char *)id->subnqn, sizeof(id->subnqn), (char*)subsys->subnqn);
>       }
> @@ -6712,8 +6717,10 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>   
>       id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
>       id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
> -    strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' ');
> -    strpadcpy((char *)id->fr, sizeof(id->fr), QEMU_VERSION, ' ');
> +    strpadcpy((char *)id->mn, sizeof(id->mn),
> +            n->params.model ? n->params.model : "QEMU NVMe Ctrl", ' ');
> +    strpadcpy((char *)id->fr, sizeof(id->fr),
> +            n->params.firmware ? n->params.firmware : QEMU_VERSION, ' ');
>       strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' ');
>   
>       id->cntlid = cpu_to_le16(n->cntlid);
> @@ -6913,6 +6920,9 @@ static Property nvme_props[] = {
>       DEFINE_PROP_LINK("subsys", NvmeCtrl, subsys, TYPE_NVME_SUBSYS,
>                        NvmeSubsystem *),
>       DEFINE_PROP_STRING("serial", NvmeCtrl, params.serial),
> +    DEFINE_PROP_STRING("model", NvmeCtrl, params.model),
> +    DEFINE_PROP_STRING("nqn_override", NvmeCtrl, params.nqn_override),
> +    DEFINE_PROP_STRING("firmware", NvmeCtrl, params.firmware),
>       DEFINE_PROP_UINT32("cmb_size_mb", NvmeCtrl, params.cmb_size_mb, 0),
>       DEFINE_PROP_UINT32("num_queues", NvmeCtrl, params.num_queues, 0),
>       DEFINE_PROP_UINT32("max_ioqpairs", NvmeCtrl, params.max_ioqpairs, 64),
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index e41771604f..45bcf3e02e 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -394,6 +394,9 @@ typedef struct NvmeCQueue {
>   
>   typedef struct NvmeParams {
>       char     *serial;
> +    char     *model;
> +    char     *firmware;
> +    char     *nqn_override;
>       uint32_t num_queues; /* deprecated since 5.1 */
>       uint32_t max_ioqpairs;
>       uint16_t msix_qsize;

[-- Attachment #2: Type: text/html, Size: 4798 bytes --]

Re: [PATCH] hw/nvme: Add options to override hardcoded values

[Keith Busch]

On Sun, Jun 12, 2022 at 12:35:09AM +0200, Mauricio Sandt wrote:
> This small patch is the result of some recent malware research I did
> in a QEMU VM. The malware used multiple ways of querying info from
> the VM disk and I needed a clean way to change those values from the
> hypervisor.
> 
> I believe this functionality could be useful to more people from multiple
> fields, sometimes you just want to change some default values and having
> them hardcoded in the sourcecode makes that much harder.
> 
> This patch adds three config parameters to the nvme device, all of them
> are optional to not break anything. If any of them are not specified,
> the previous (before this patch) default is used.
> 
> -model - This takes a string and sets it as the devices model name.
> If you don't specify this parameter, the default is "QEMU NVMe Ctrl".
> 
> -firmware - The firmware version string, max 8 ascii characters.
> The default is whatever `QEMU_VERSION` evaluates to.
> 
> -nqn_override - Allows to set a custom nqn on the nvme device.
> Only used if there is no subsystem. This string should be in the same
> format as the default "nqn.2019-08.org.qemu:...", but there is no
> validation for that. Its up to the user to provide a valid string.

I guess the nqn can be user tunable just like it is when used with subsystems,
but what's the point of messing with model and firmware? That could mess with
host drivers' ability to detect what quirks it needs to apply to specific
instances of this virtual controller.

Re: [PATCH] hw/nvme: Add options to override hardcoded values

[Mauricio Sandt]

I want to argue the other way around. Why shouldn't those values
be tunable by the user? You are right; if misconfigured, it could 
potentially
break stuff on the driver side, but unless you manually set values for model
and firmware, the default is used (just like it is now), so this patch
wouldn't break any existing setups.
In my opinion, having more freedom in the values being used for model names
and similar is much better than hardcoding values. The previously hardcoded
values should remain the default if no custom value is provided.
My specific use case that required this patch is a piece of malware that 
used
several IOCTLs to read model, firmware, and nqn from the NVMe attached to
the VM. Modifying that info at the hypervisor level was a much better 
approach
than loading an (unsigned) driver into windows to intercept said IOCTLs.
Having this patch in the official qemu version would help me a lot in my 
test
lab, and I'm pretty sure it would also help other people.

I guess there could be a warning about potential problems with drivers 
in the
description for model, firmware, and nqn, but I haven't experienced any 
issues
when changing the values (for all of them). If you encountered any problems,
how did they manifest?

On 13/07/2022 19:03, Keith Busch wrote:
> On Sun, Jun 12, 2022 at 12:35:09AM +0200, Mauricio Sandt wrote:
>> This small patch is the result of some recent malware research I did
>> in a QEMU VM. The malware used multiple ways of querying info from
>> the VM disk and I needed a clean way to change those values from the
>> hypervisor.
>>
>> I believe this functionality could be useful to more people from multiple
>> fields, sometimes you just want to change some default values and having
>> them hardcoded in the sourcecode makes that much harder.
>>
>> This patch adds three config parameters to the nvme device, all of them
>> are optional to not break anything. If any of them are not specified,
>> the previous (before this patch) default is used.
>>
>> -model - This takes a string and sets it as the devices model name.
>> If you don't specify this parameter, the default is "QEMU NVMe Ctrl".
>>
>> -firmware - The firmware version string, max 8 ascii characters.
>> The default is whatever `QEMU_VERSION` evaluates to.
>>
>> -nqn_override - Allows to set a custom nqn on the nvme device.
>> Only used if there is no subsystem. This string should be in the same
>> format as the default "nqn.2019-08.org.qemu:...", but there is no
>> validation for that. Its up to the user to provide a valid string.
> I guess the nqn can be user tunable just like it is when used with subsystems,
> but what's the point of messing with model and firmware? That could mess with
> host drivers' ability to detect what quirks it needs to apply to specific
> instances of this virtual controller.

Re: [PATCH] hw/nvme: Add options to override hardcoded values

[Keith Busch]

On Wed, Jul 13, 2022 at 08:06:26PM +0200, Mauricio Sandt wrote:
> My specific use case that required this patch is a piece of malware that used
> several IOCTLs to read model, firmware, and nqn from the NVMe attached to the
> VM. Modifying that info at the hypervisor level was a much better approach
> than loading an (unsigned) driver into windows to intercept said IOCTLs.
> Having this patch in the official qemu version would help me a lot in my test
> lab, and I'm pretty sure it would also help other people.

I guess I'm missing the bigger picture here. You are supposed to be able to
retrieve these fields with ioctl's, so not sure what this has to do with
malware. Why does the firmware revision matter to this program?
 
> I guess there could be a warning about potential problems with drivers in the
> description for model, firmware, and nqn, but I haven't experienced any
> issues when changing the values (for all of them). If you encountered any
> problems, how did they manifest?

Older qemu nvme wasn't reliably generating unique identifiers, so linux quirks
them to ignore. They are reliable now, so the quirk can be changed to firmware
specific when the version number updates with the next release.

Re: [PATCH] hw/nvme: Add options to override hardcoded values

[Mauricio Sandt]

On 13/07/2022 20:48, Keith Busch wrote:
> I guess I'm missing the bigger picture here. You are supposed to be able to
> retrieve these fields with ioctl's, so not sure what this has to do with
> malware. Why does the firmware revision matter to this program?
Oh I'm sorry, I forgot to explain properly. Malware usually checks if it is
being run in a sandbox environment like a VM, and if it detects such a
sandbox, it doesn't run or doesn't unleash its full potential. This makes my
life as a researcher much harder.

Hiding the VM by overriding the model, firmware, and nqn strings to either
random values or names of existing hardware in the hypervisor is a much
cleaner solution than intercepting the IOCTLs in the VM and changing the
result with a kernel driver.


> Older qemu nvme wasn't reliably generating unique identifiers, so linux quirks
> them to ignore. They are reliable now, so the quirk can be changed to firmware
> specific when the version number updates with the next release.
If I understand you correctly, that means that changing the model and
firmware version to something that doesn't identify the device as a
qemu device would work just fine provided that you're running a recent
qemu version?

Re: [PATCH] hw/nvme: Add options to override hardcoded values

[Keith Busch]

On Wed, Jul 13, 2022 at 09:11:41PM +0200, Mauricio Sandt wrote:
> On 13/07/2022 20:48, Keith Busch wrote:
> > I guess I'm missing the bigger picture here. You are supposed to be able to
> > retrieve these fields with ioctl's, so not sure what this has to do with
> > malware. Why does the firmware revision matter to this program?
> Oh I'm sorry, I forgot to explain properly. Malware usually checks if it is
> being run in a sandbox environment like a VM, and if it detects such a
> sandbox, it doesn't run or doesn't unleash its full potential. This makes my
> life as a researcher much harder.
> 
> Hiding the VM by overriding the model, firmware, and nqn strings to either
> random values or names of existing hardware in the hypervisor is a much
> cleaner solution than intercepting the IOCTLs in the VM and changing the
> result with a kernel driver.

IIUC, this program is trying to avoid being studied, and uses indicators like
nvme firmware to help determine if it is running in such an environment. If so,
I suspect defeating all possible indicators will be a fun and time consuming
process. :)