[Qemu-devel] [PATCH v1] i2c: pm_smbus: check smb_index before block transfer write

P J P posted 1 patch 5 days ago
Test checkpatch passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181206121830.6177-1-ppandit@redhat.com
hw/i2c/pm_smbus.c | 3 +++
1 file changed, 3 insertions(+)

[Qemu-devel] [PATCH v1] i2c: pm_smbus: check smb_index before block transfer write

Posted by P J P 5 days ago
From: Prasad J Pandit <pjp@fedoraproject.org>

While performing block transfer write in smb_ioport_writeb(),
'smb_index' is incremented and used to index smb_data[] array.
Check 'smb_index' value to avoid OOB access.

Note that this bug is exploitable by a guest to escape
from the virtual machine. However the commit which
introduced the bug was only made after the 3.0 release,
and so it is not present in any released QEMU versions.

Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability
Reported-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/i2c/pm_smbus.c | 3 +++
 1 file changed, 3 insertions(+)

Update v1: add note about issue being introduced after 3.0 release
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01115.html

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 685a2378ed..03062740cc 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -240,6 +240,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
             uint8_t read = s->smb_addr & 0x01;
 
             s->smb_index++;
+            if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
+                s->smb_index = 0;
+            }
             if (!read && s->smb_index == s->smb_data0) {
                 uint8_t prot = (s->smb_ctl >> 2) & 0x07;
                 uint8_t cmd = s->smb_cmd;
-- 
2.19.2


Re: [Qemu-devel] [PATCH v1] i2c: pm_smbus: check smb_index before block transfer write

Posted by Peter Maydell 5 days ago
On Thu, 6 Dec 2018 at 12:20, P J P <ppandit@redhat.com> wrote:
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While performing block transfer write in smb_ioport_writeb(),
> 'smb_index' is incremented and used to index smb_data[] array.
> Check 'smb_index' value to avoid OOB access.
>
> Note that this bug is exploitable by a guest to escape
> from the virtual machine. However the commit which
> introduced the bug was only made after the 3.0 release,
> and so it is not present in any released QEMU versions.
>
> Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/i2c/pm_smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> Update v1: add note about issue being introduced after 3.0 release
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01115.html

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v1] i2c: pm_smbus: check smb_index before block transfer write

Posted by Michael S. Tsirkin 5 days ago
On Thu, Dec 06, 2018 at 05:48:30PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While performing block transfer write in smb_ioport_writeb(),
> 'smb_index' is incremented and used to index smb_data[] array.
> Check 'smb_index' value to avoid OOB access.
> 
> Note that this bug is exploitable by a guest to escape
> from the virtual machine. However the commit which
> introduced the bug was only made after the 3.0 release,
> and so it is not present in any released QEMU versions.
> 
> Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/i2c/pm_smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> Update v1: add note about issue being introduced after 3.0 release
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01115.html
> 
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 685a2378ed..03062740cc 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -240,6 +240,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>              uint8_t read = s->smb_addr & 0x01;
>  
>              s->smb_index++;
> +            if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
> +                s->smb_index = 0;
> +            }

when I see patterns like these I always think "what about spectre".
But not a new issue and not the only place, so we can leave this
for another day.

>              if (!read && s->smb_index == s->smb_data0) {
>                  uint8_t prot = (s->smb_ctl >> 2) & 0x07;
>                  uint8_t cmd = s->smb_cmd;
> -- 
> 2.19.2

Re: [Qemu-devel] [PATCH v1] i2c: pm_smbus: check smb_index before block transfer write

Posted by Philippe Mathieu-Daudé 5 days ago
On 12/6/18 1:18 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While performing block transfer write in smb_ioport_writeb(),
> 'smb_index' is incremented and used to index smb_data[] array.
> Check 'smb_index' value to avoid OOB access.
> 
> Note that this bug is exploitable by a guest to escape
> from the virtual machine. However the commit which
> introduced the bug was only made after the 3.0 release,
> and so it is not present in any released QEMU versions.
> 
> Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/i2c/pm_smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> Update v1: add note about issue being introduced after 3.0 release
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01115.html
> 
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 685a2378ed..03062740cc 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -240,6 +240,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>              uint8_t read = s->smb_addr & 0x01;
>  
>              s->smb_index++;
> +            if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
> +                s->smb_index = 0;
> +            }
>              if (!read && s->smb_index == s->smb_data0) {
>                  uint8_t prot = (s->smb_ctl >> 2) & 0x07;
>                  uint8_t cmd = s->smb_cmd;
>