: hw/i386/vmport: Bug fixes and improvements

[PATCH 14/14] hw/i386/vmport: Assert vmport initialized before registering commands

Posted by Liran Alon 5 years, 11 months ago

vmport_register() is also called from other modules such as vmmouse.
Therefore, these modules rely that vmport is realized before those call
sites. If this is violated, vmport_register() will NULL-deref.

To make such issues easier to debug, assert in vmport_register() that
vmport is already realized.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 95d4a23ce9ba..659a323e8448 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -68,6 +68,8 @@ static VMPortState *port_state;
 void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
 {
     assert(command < VMPORT_ENTRIES);
+    assert(port_state);
+
     trace_vmport_register(command, func, opaque);
     port_state->func[command] = func;
     port_state->opaque[command] = opaque;
-- 
2.20.1

Re: [PATCH 14/14] hw/i386/vmport: Assert vmport initialized before registering commands

Posted by Michael S. Tsirkin 5 years, 11 months ago

On Tue, Mar 10, 2020 at 01:54:11AM +0200, Liran Alon wrote:
> vmport_register() is also called from other modules such as vmmouse.
> Therefore, these modules rely that vmport is realized before those call
> sites. If this is violated, vmport_register() will NULL-deref.
> 
> To make such issues easier to debug, assert in vmport_register() that
> vmport is already realized.
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>


Hmm and what does actually make sure it is realized?

> ---
>  hw/i386/vmport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index 95d4a23ce9ba..659a323e8448 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -68,6 +68,8 @@ static VMPortState *port_state;
>  void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
>  {
>      assert(command < VMPORT_ENTRIES);
> +    assert(port_state);
> +
>      trace_vmport_register(command, func, opaque);
>      port_state->func[command] = func;
>      port_state->opaque[command] = opaque;
> -- 
> 2.20.1

Re: [PATCH 14/14] hw/i386/vmport: Assert vmport initialized before registering commands

Posted by Liran Alon 5 years, 11 months ago

On 10/03/2020 11:30, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:54:11AM +0200, Liran Alon wrote:
>> vmport_register() is also called from other modules such as vmmouse.
>> Therefore, these modules rely that vmport is realized before those call
>> sites. If this is violated, vmport_register() will NULL-deref.
>>
>> To make such issues easier to debug, assert in vmport_register() that
>> vmport is already realized.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>
> Hmm and what does actually make sure it is realized?

port_state global var is only set in vmport_realizefn().

-Liran

>
>> ---
>>   hw/i386/vmport.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
>> index 95d4a23ce9ba..659a323e8448 100644
>> --- a/hw/i386/vmport.c
>> +++ b/hw/i386/vmport.c
>> @@ -68,6 +68,8 @@ static VMPortState *port_state;
>>   void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
>>   {
>>       assert(command < VMPORT_ENTRIES);
>> +    assert(port_state);
>> +
>>       trace_vmport_register(command, func, opaque);
>>       port_state->func[command] = func;
>>       port_state->opaque[command] = opaque;
>> -- 
>> 2.20.1