1. Patchew
  2. QEMU
  3. [PATCH v4 0/7] Enable PC diversion via the plugin API
  4. View patch

[PATCH v4 6/7] plugins: prohibit writing to read-only registers

Florian Hofhammer posted 7 patches 1 month, 2 weeks ago
Maintainers: Laurent Vivier <laurent@vivier.eu>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Brian Cain <brian.cain@oss.qualcomm.com>, "Alex Bennée" <alex.bennee@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <arikalo@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>
There is a newer version of this series
[PATCH v4 6/7] plugins: prohibit writing to read-only registers
Posted by Florian Hofhammer 1 month, 2 weeks ago 
The opaque register handle encodes whether a register is read-only in
the lowest bit and prevents writing to the register via the plugin API
in this case.

Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
---
 plugins/api.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/plugins/api.c b/plugins/api.c
index b2c52d2a09..3a8479ddce 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -424,6 +424,7 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
     for (int i = 0; i < gdbstub_regs->len; i++) {
         GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
         qemu_plugin_reg_descriptor desc;
+        gint plugin_ro_bit = 0;
 
         /* skip "un-named" regs */
         if (!grd->name) {
@@ -431,7 +432,6 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
         }
 
         /* Create a record for the plugin */
-        desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1);
         desc.name = g_intern_string(grd->name);
         desc.is_readonly = false;
         if (g_strcmp0(desc.name, pc_str) == 0
@@ -442,7 +442,9 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
             || g_strcmp0(desc.name, rpc_str) == 0
            ) {
             desc.is_readonly = true;
+            plugin_ro_bit = 1;
         }
+        desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit);
         desc.feature = g_intern_string(grd->feature_name);
         g_array_append_val(find_data, desc);
     }
@@ -467,7 +469,7 @@ bool qemu_plugin_read_register(struct qemu_plugin_register *reg,
         return false;
     }
 
-    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1) > 0);
+    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) > 0);
 }
 
 bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
@@ -476,11 +478,12 @@ bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
     g_assert(current_cpu);
 
     if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&
-                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)) {
+                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)
+        || (GPOINTER_TO_INT(reg) & 1)) {
         return false;
     }
 
-    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1) > 0);
+    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) >> 1) > 0);
 }
 
 void qemu_plugin_set_pc(uint64_t vaddr)
-- 
2.53.0
Re: [PATCH v4 6/7] plugins: prohibit writing to read-only registers
Posted by Alex Bennée 1 month, 2 weeks ago 
Florian Hofhammer <florian.hofhammer@epfl.ch> writes:

> The opaque register handle encodes whether a register is read-only in
> the lowest bit and prevents writing to the register via the plugin API
> in this case.
>
> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
> ---
>  plugins/api.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/api.c b/plugins/api.c
> index b2c52d2a09..3a8479ddce 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -424,6 +424,7 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>      for (int i = 0; i < gdbstub_regs->len; i++) {
>          GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
>          qemu_plugin_reg_descriptor desc;
> +        gint plugin_ro_bit = 0;
>  
>          /* skip "un-named" regs */
>          if (!grd->name) {
> @@ -431,7 +432,6 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>          }
>  
>          /* Create a record for the plugin */
> -        desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1);
>          desc.name = g_intern_string(grd->name);
>          desc.is_readonly = false;
>          if (g_strcmp0(desc.name, pc_str) == 0
> @@ -442,7 +442,9 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>              || g_strcmp0(desc.name, rpc_str) == 0
>             ) {
>              desc.is_readonly = true;
> +            plugin_ro_bit = 1;
>          }
> +        desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit);
>          desc.feature = g_intern_string(grd->feature_name);
>          g_array_append_val(find_data, desc);
>      }
> @@ -467,7 +469,7 @@ bool qemu_plugin_read_register(struct qemu_plugin_register *reg,
>          return false;
>      }
>  
> -    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1) > 0);
> +    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) > 0);
>  }
>  
>  bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
> @@ -476,11 +478,12 @@ bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>      g_assert(current_cpu);
>  
>      if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&
> -                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)) {
> +                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)
> +        || (GPOINTER_TO_INT(reg) & 1)) {

Maybe this is better as:

      g_assert(GPOINTER_TO_INT(reg) & 1 == 0);

      if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&...

again the plugin is trying to do something it shouldn't.

>          return false;
>      }
>  
> -    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1) > 0);
> +    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) >> 1) > 0);
>  }
>  
>  void qemu_plugin_set_pc(uint64_t vaddr)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v4 6/7] plugins: prohibit writing to read-only registers
Posted by Florian Hofhammer 1 month, 1 week ago 
On 24/02/2026 18:49, Alex Bennée wrote:
> Florian Hofhammer <florian.hofhammer@epfl.ch> writes:
> 
>> The opaque register handle encodes whether a register is read-only in
>> the lowest bit and prevents writing to the register via the plugin API
>> in this case.
>>
>> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
>> ---
>>  plugins/api.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/plugins/api.c b/plugins/api.c
>> index b2c52d2a09..3a8479ddce 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -424,6 +424,7 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>      for (int i = 0; i < gdbstub_regs->len; i++) {
>>          GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
>>          qemu_plugin_reg_descriptor desc;
>> +        gint plugin_ro_bit = 0;
>>  
>>          /* skip "un-named" regs */
>>          if (!grd->name) {
>> @@ -431,7 +432,6 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>          }
>>  
>>          /* Create a record for the plugin */
>> -        desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1);
>>          desc.name = g_intern_string(grd->name);
>>          desc.is_readonly = false;
>>          if (g_strcmp0(desc.name, pc_str) == 0
>> @@ -442,7 +442,9 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>              || g_strcmp0(desc.name, rpc_str) == 0
>>             ) {
>>              desc.is_readonly = true;
>> +            plugin_ro_bit = 1;
>>          }
>> +        desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit);
>>          desc.feature = g_intern_string(grd->feature_name);
>>          g_array_append_val(find_data, desc);
>>      }
>> @@ -467,7 +469,7 @@ bool qemu_plugin_read_register(struct qemu_plugin_register *reg,
>>          return false;
>>      }
>>  
>> -    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1) > 0);
>> +    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) > 0);
>>  }
>>  
>>  bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>> @@ -476,11 +478,12 @@ bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>>      g_assert(current_cpu);
>>  
>>      if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&
>> -                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)) {
>> +                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)
>> +        || (GPOINTER_TO_INT(reg) & 1)) {
> 
> Maybe this is better as:
> 
>       g_assert(GPOINTER_TO_INT(reg) & 1 == 0);
> 
>       if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&...
> 
> again the plugin is trying to do something it shouldn't.

As far as I can tell, there's currently no mechanism in the test setup
to check that a certain assertion is triggered. In the previous test, I
just checked whether the API returns false when I try to write to a
read-only register, but now I'd need to check in my test whether the
assert triggers.

Should I check that in a wrapper script, test only the read path, or
follow a completely different approach for this?

> 
>>          return false;
>>      }
>>  
>> -    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1) > 0);
>> +    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) >> 1) > 0);
>>  }
>>  
>>  void qemu_plugin_set_pc(uint64_t vaddr)
> 

Best regards,
Florian
Re: [PATCH v4 6/7] plugins: prohibit writing to read-only registers
Posted by Alex Bennée 1 month, 1 week ago 
Florian Hofhammer <florian.hofhammer@epfl.ch> writes:

> On 24/02/2026 18:49, Alex Bennée wrote:
>> Florian Hofhammer <florian.hofhammer@epfl.ch> writes:
>> 
>>> The opaque register handle encodes whether a register is read-only in
>>> the lowest bit and prevents writing to the register via the plugin API
>>> in this case.
>>>
>>> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
>>> ---
>>>  plugins/api.c | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/plugins/api.c b/plugins/api.c
>>> index b2c52d2a09..3a8479ddce 100644
>>> --- a/plugins/api.c
>>> +++ b/plugins/api.c
>>> @@ -424,6 +424,7 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>>      for (int i = 0; i < gdbstub_regs->len; i++) {
>>>          GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
>>>          qemu_plugin_reg_descriptor desc;
>>> +        gint plugin_ro_bit = 0;
>>>  
>>>          /* skip "un-named" regs */
>>>          if (!grd->name) {
>>> @@ -431,7 +432,6 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>>          }
>>>  
>>>          /* Create a record for the plugin */
>>> -        desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1);
>>>          desc.name = g_intern_string(grd->name);
>>>          desc.is_readonly = false;
>>>          if (g_strcmp0(desc.name, pc_str) == 0
>>> @@ -442,7 +442,9 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>>              || g_strcmp0(desc.name, rpc_str) == 0
>>>             ) {
>>>              desc.is_readonly = true;
>>> +            plugin_ro_bit = 1;
>>>          }
>>> +        desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit);
>>>          desc.feature = g_intern_string(grd->feature_name);
>>>          g_array_append_val(find_data, desc);
>>>      }
>>> @@ -467,7 +469,7 @@ bool qemu_plugin_read_register(struct qemu_plugin_register *reg,
>>>          return false;
>>>      }
>>>  
>>> -    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1) > 0);
>>> +    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) > 0);
>>>  }
>>>  
>>>  bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>>> @@ -476,11 +478,12 @@ bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>>>      g_assert(current_cpu);
>>>  
>>>      if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&
>>> -                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)) {
>>> +                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)
>>> +        || (GPOINTER_TO_INT(reg) & 1)) {
>> 
>> Maybe this is better as:
>> 
>>       g_assert(GPOINTER_TO_INT(reg) & 1 == 0);
>> 
>>       if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&...
>> 
>> again the plugin is trying to do something it shouldn't.
>
> As far as I can tell, there's currently no mechanism in the test setup
> to check that a certain assertion is triggered. In the previous test, I
> just checked whether the API returns false when I try to write to a
> read-only register, but now I'd need to check in my test whether the
> assert triggers.

Ahh I see - I was going to say we don't really need to test for abuse of
the API triggering asserts but you next test does indeed do that. I
think we can live without explicitly adding a test case for attempts to
write to read only registers.

>
> Should I check that in a wrapper script, test only the read path, or
> follow a completely different approach for this?
>
>> 
>>>          return false;
>>>      }
>>>  
>>> -    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1) > 0);
>>> +    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) >> 1) > 0);
>>>  }
>>>  
>>>  void qemu_plugin_set_pc(uint64_t vaddr)
>> 
>
> Best regards,
> Florian

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v4 6/7] plugins: prohibit writing to read-only registers
Posted by Florian Hofhammer 1 month, 1 week ago 
On 02/03/2026 14:03, Alex Bennée wrote:
> Florian Hofhammer <florian.hofhammer@epfl.ch> writes:
> 
>> On 24/02/2026 18:49, Alex Bennée wrote:
>>> Florian Hofhammer <florian.hofhammer@epfl.ch> writes:
>>>
>>>> The opaque register handle encodes whether a register is read-only in
>>>> the lowest bit and prevents writing to the register via the plugin API
>>>> in this case.
>>>>
>>>> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
>>>> ---
>>>>  plugins/api.c | 11 +++++++----
>>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/plugins/api.c b/plugins/api.c
>>>> index b2c52d2a09..3a8479ddce 100644
>>>> --- a/plugins/api.c
>>>> +++ b/plugins/api.c
>>>> @@ -424,6 +424,7 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>>>      for (int i = 0; i < gdbstub_regs->len; i++) {
>>>>          GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
>>>>          qemu_plugin_reg_descriptor desc;
>>>> +        gint plugin_ro_bit = 0;
>>>>  
>>>>          /* skip "un-named" regs */
>>>>          if (!grd->name) {
>>>> @@ -431,7 +432,6 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>>>          }
>>>>  
>>>>          /* Create a record for the plugin */
>>>> -        desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1);
>>>>          desc.name = g_intern_string(grd->name);
>>>>          desc.is_readonly = false;
>>>>          if (g_strcmp0(desc.name, pc_str) == 0
>>>> @@ -442,7 +442,9 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>>>              || g_strcmp0(desc.name, rpc_str) == 0
>>>>             ) {
>>>>              desc.is_readonly = true;
>>>> +            plugin_ro_bit = 1;
>>>>          }
>>>> +        desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit);
>>>>          desc.feature = g_intern_string(grd->feature_name);
>>>>          g_array_append_val(find_data, desc);
>>>>      }
>>>> @@ -467,7 +469,7 @@ bool qemu_plugin_read_register(struct qemu_plugin_register *reg,
>>>>          return false;
>>>>      }
>>>>  
>>>> -    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1) > 0);
>>>> +    return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) > 0);
>>>>  }
>>>>  
>>>>  bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>>>> @@ -476,11 +478,12 @@ bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
>>>>      g_assert(current_cpu);
>>>>  
>>>>      if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&
>>>> -                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)) {
>>>> +                          qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS_PC)
>>>> +        || (GPOINTER_TO_INT(reg) & 1)) {
>>>
>>> Maybe this is better as:
>>>
>>>       g_assert(GPOINTER_TO_INT(reg) & 1 == 0);
>>>
>>>       if (buf->len == 0 || (qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS &&...
>>>
>>> again the plugin is trying to do something it shouldn't.
>>
>> As far as I can tell, there's currently no mechanism in the test setup
>> to check that a certain assertion is triggered. In the previous test, I
>> just checked whether the API returns false when I try to write to a
>> read-only register, but now I'd need to check in my test whether the
>> assert triggers.
> 
> Ahh I see - I was going to say we don't really need to test for abuse of
> the API triggering asserts but you next test does indeed do that. I
> think we can live without explicitly adding a test case for attempts to
> write to read only registers.

Ack, thanks for the quick reply!

> 
>>
>> Should I check that in a wrapper script, test only the read path, or
>> follow a completely different approach for this?
>>
>>>
>>>>          return false;
>>>>      }
>>>>  
>>>> -    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1) > 0);
>>>> +    return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) >> 1) > 0);
>>>>  }
>>>>  
>>>>  void qemu_plugin_set_pc(uint64_t vaddr)
>>>
>>
>> Best regards,
>> Florian
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.