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

["Alex Bennée" <alex.bennee@linaro.org>]

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