[Qemu-devel] [PATCH v2 for v2.3.0] fw_cfg: add check to validate current entry value

[Qemu-devel] [PATCH v2 for v2.3.0] fw_cfg: add check to validate current entry value

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

When processing firmware configurations, an OOB r/w access occurs
if 's->cur_entry' is set to be invalid(FW_CFG_INVALID=0xffff).
Add a check to validate 's->cur_entry' to avoid such access.

Reported-by: Donghai Zdh <donghai.zdh@alibaba-inc.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/nvram/fw_cfg.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Updated as per review in
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00398.html

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 68eff77..e73c0fb 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -233,12 +233,15 @@ static void fw_cfg_reboot(FWCfgState *s)
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
 {
     int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+    FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
+                     &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
 
     trace_fw_cfg_write(s, value);
 
-    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
-        s->cur_offset < e->len) {
+    if (s->cur_entry & FW_CFG_WRITE_CHANNEL
+        && e != NULL
+        && e->callback
+        && s->cur_offset < e->len) {
         e->data[s->cur_offset++] = value;
         if (s->cur_offset == e->len) {
             e->callback(e->callback_opaque, e->data);
@@ -267,7 +270,8 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
 static uint8_t fw_cfg_read(FWCfgState *s)
 {
     int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+    FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
+                     &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
     uint8_t ret;
 
     if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 for v2.3.0] fw_cfg: add check to validate current entry value

[P J P]

  Hello,

+-- On Wed, 6 Jan 2016, P J P wrote --+
| When processing firmware configurations, an OOB r/w access occurs
| if 's->cur_entry' is set to be invalid(FW_CFG_INVALID=0xffff).
| Add a check to validate 's->cur_entry' to avoid such access.
| 
| Reported-by: Donghai Zdh <donghai.zdh@alibaba-inc.com>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  hw/nvram/fw_cfg.c | 12 ++++++++----
|  1 file changed, 8 insertions(+), 4 deletions(-)
| 
| Updated as per review in
|   -> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00398.html


  -> https://patchwork.ozlabs.org/patch/563706/

Ping...! Does it look okay? (just checking)

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 for v2.3.0] fw_cfg: add check to validate current entry value

[Paolo Bonzini]

On 11/01/2016 08:23, P J P wrote:
>   Hello,
> 
> +-- On Wed, 6 Jan 2016, P J P wrote --+
> | When processing firmware configurations, an OOB r/w access occurs
> | if 's->cur_entry' is set to be invalid(FW_CFG_INVALID=0xffff).
> | Add a check to validate 's->cur_entry' to avoid such access.
> | 
> | Reported-by: Donghai Zdh <donghai.zdh@alibaba-inc.com>
> | Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> | ---
> |  hw/nvram/fw_cfg.c | 12 ++++++++----
> |  1 file changed, 8 insertions(+), 4 deletions(-)
> | 
> | Updated as per review in
> |   -> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00398.html
> 
> 
>   -> https://patchwork.ozlabs.org/patch/563706/
> 
> Ping...! Does it look okay? (just checking)

Yes, it looks good.

Paolo

Re: [Qemu-devel] [PATCH v2 for v2.3.0] fw_cfg: add check to validate current entry value

[P J P]

+-- On Mon, 11 Jan 2016, Paolo Bonzini wrote --+
| > +-- On Wed, 6 Jan 2016, P J P wrote --+
| >   -> https://patchwork.ozlabs.org/patch/563706/
| > Ping...! Does it look okay? (just checking)
| 
| Yes, it looks good.

Okay, thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F