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

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

[P J P]

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

Hello,

An OOB r/w access issue was reported by Mr Donghai Zdh, CC'd here. It occurs
while processing firmware configurations in Qemu versions prior to 2.4. The
OOB memory access crashes the Qemu process on the host.

Please see below a (tested)patch to fix this issue. Does it look okay?

Thank you!

Prasad J Pandit (1):
  fw_cfg: add check to validate current entry value

 hw/nvram/fw_cfg.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

--
2.4.3

[Qemu-devel] [PATCH 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(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 68eff77..ce026bc 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_INVALID
+        && s->cur_entry & FW_CFG_WRITE_CHANNEL
+        && 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 for v2.3.0] fw_cfg: add check to validate current entry value

[P J P]

+-- On Tue, 5 Jan 2016, P J P wrote --+
| An OOB r/w access issue was reported by Mr Donghai Zdh, CC'd here.

Mr Donghai CC'd now.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Stefan Weil]

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

Am 05.01.2016 um 15:55 schrieb 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(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 68eff77..ce026bc 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_INVALID
> +        && s->cur_entry & FW_CFG_WRITE_CHANNEL
> +        && e->callback
> +        && s->cur_offset < e->len) {

I suggest to test e != NULL instead of s->cur_entry != FW_CFG_INVALID.

Of course both variants are equivalent, but e != NULL might be easier
to review and make work of static code analyzers easier, too.


>          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)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[朱东海(启路)]

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

Hi, Will you assign a cve to this vulnerability.This issue has the possibility to remote code execution, and many IAAS providers use qemu prior version 2.4.Donghai.------------------------------------------------------------------From:P J P <ppandit@redhat.com>Send Time:2016年1月6日(星期三) 02:08To:Qemu devel <qemu-devel@nongnu.org>Cc:Stefan Weil <sw@weilnetz.de>，Peter Maydell <peter.maydell@linaro.org>，朱东海(启路) <donghai.zdh@alibaba-inc.com>Subject:Re: [Qemu-devel] [PATCH for v2.3.0] fw_cfg: add check to validate current entry value+-- On Tue, 5 Jan 2016, P J P wrote --+| An OOB r/w access issue was reported by Mr Donghai Zdh, CC'd here.Mr Donghai CC'd now.--Prasad J Pandit / Red Hat Product Security Team47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

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

[P J P]

+-- On Tue, 5 Jan 2016, Stefan Weil wrote --+
| > -        s->cur_offset < e->len) {
| > +    if (s->cur_entry != FW_CFG_INVALID
| > +        && s->cur_entry & FW_CFG_WRITE_CHANNEL
| > +        && e->callback
| > +        && s->cur_offset < e->len) {
| 
| I suggest to test e != NULL instead of s->cur_entry != FW_CFG_INVALID.
| 
| Of course both variants are equivalent, but e != NULL might be easier
| to review and make work of static code analyzers easier, too.

  Yes, I've sent a revised patch with this change.

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

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

[P J P]

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

+-- On Wed, 6 Jan 2016, 朱东海(启路) wrote --+
| Hi, Will you assign a cve to this vulnerability.

Yes, I will once the patch is approved upstream. 
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F