[for,v2.3.0] fw_cfg: add check to validate current entry value

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

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

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