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

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

[P J P]

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.

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

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] i2c: pm_smbus: check smb_index before block transfer write

[li qiang]

在 2018/12/6 16:48, P J P 写道:
> 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.
>
> 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(+)
>
> 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;

Oh... Finally another one find this.....

I've already found this. This is very a serious security issue.

I have wrote a full exploit to make a VM escape using this vulnerability.

This guest can read/write a 4G memory of qemu process by default 
configuration.

As far as I know, this vulnerability may be the most serious 
vulnerability of the qemu history.

Please pay a lot of attention for this issue.

Later I will release the full paper and exploit. It's not harm as this 
is introduced in 3.1

and no one use it now.


Thanks,

Li Qiang

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

[li qiang]

FYI:

http://terenceli.github.io/%E6%8A%80%E6%9C%AF/2018/12/06/qemu-escape


在 2018/12/6 17:02, li qiang 写道:
> 在 2018/12/6 16:48, P J P 写道:
>> 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.
>>
>> 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(+)
>>
>> 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;
> Oh... Finally another one find this.....
>
> I've already found this. This is very a serious security issue.
>
> I have wrote a full exploit to make a VM escape using this vulnerability.
>
> This guest can read/write a 4G memory of qemu process by default
> configuration.
>
> As far as I know, this vulnerability may be the most serious
> vulnerability of the qemu history.
>
> Please pay a lot of attention for this issue.
>
> Later I will release the full paper and exploit. It's not harm as this
> is introduced in 3.1
>
> and no one use it now.
>
>
> Thanks,
>
> Li Qiang
>
>
>
>

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 09:10, li qiang <liq3ea@outlook.com> wrote:
> Oh... Finally another one find this.....
>
> I've already found this. This is very a serious security issue.

If you find a security issue, we would appreciate it if
you let us know, rather than just waiting to see if
anybody else notices it...

thanks
-- PMM

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

[li qiang]

在 2018/12/6 18:16, Peter Maydell 写道:
> On Thu, 6 Dec 2018 at 09:10, li qiang<liq3ea@outlook.com>  wrote:
>> Oh... Finally another one find this.....
>>
>> I've already found this. This is very a serious security issue.
> If you find a security issue, we would appreciate it if
> you let us know, rather than just waiting to see if
> anybody else notices it...

I'm sorry for that.

To be perfect, I'm currently trying to write an exp for i440fx.

I intend report it after completing it.

Thanks,
Li Qiang


> thanks
> -- PMM

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 10:34, li qiang <liq3ea@outlook.com> wrote:
>
>
> 在 2018/12/6 18:16, Peter Maydell 写道:
> > On Thu, 6 Dec 2018 at 09:10, li qiang<liq3ea@outlook.com>  wrote:
> >> Oh... Finally another one find this.....
> >>
> >> I've already found this. This is very a serious security issue.
> > If you find a security issue, we would appreciate it if
> > you let us know, rather than just waiting to see if
> > anybody else notices it...
>
> I'm sorry for that.
>
> To be perfect, I'm currently trying to write an exp for i440fx.
>
> I intend report it after completing it.

Is that an exploit for this bug, or for some other bug?
It's OK (and we recommend) reporting security issues as
"I think this is probably exploitable"; we don't require
a proof-of-concept for security bugs.

thanks
-- PMM

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

[Li Qiang]

Peter Maydell <peter.maydell@linaro.org> 于2018年12月6日周四 下午6:46写道：

> On Thu, 6 Dec 2018 at 10:34, li qiang <liq3ea@outlook.com> wrote:
> >
> >
> > 在 2018/12/6 18:16, Peter Maydell 写道:
> > > On Thu, 6 Dec 2018 at 09:10, li qiang<liq3ea@outlook.com>  wrote:
> > >> Oh... Finally another one find this.....
> > >>
> > >> I've already found this. This is very a serious security issue.
> > > If you find a security issue, we would appreciate it if
> > > you let us know, rather than just waiting to see if
> > > anybody else notices it...
> >
> > I'm sorry for that.
> >
> > To be perfect, I'm currently trying to write an exp for i440fx.
> >
> > I intend report it after completing it.
>
> Is that an exploit for this bug, or for some other bug?


Just for this. But for now still no exp for i440fx.


> It's OK (and we recommend) reporting security issues as
> "I think this is probably exploitable"; we don't require
> a proof-of-concept for security bugs.
>

Yes, I know that, but as this issue is so good to write a perfect exploit
so I want to do more.

I know the qemu planing and know this issue doesn't affect anyone.
I want to do a perfect work.



Thanks,
Li Qiang


>
> thanks
> -- PMM
>

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 11:00, Li Qiang <liq3ea@gmail.com> wrote:
> Yes, I know that, but as this issue is so good to write a perfect exploit
> so I want to do more.
>
> I know the qemu planing and know this issue doesn't affect anyone.
> I want to do a perfect work.

The problem is that it does affect other people, because if
Michael hadn't happened to also notice this bug then we would
have released 3.1 next Tuesday with the bug still present.
As it is we'll need to do an extra rc5, which we could have
avoided if we'd known about this bug earlier, on Monday or Tuesday.

thanks
-- PMM

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

[Li Qiang]

Peter Maydell <peter.maydell@linaro.org> 于2018年12月6日周四 下午7:05写道：

> On Thu, 6 Dec 2018 at 11:00, Li Qiang <liq3ea@gmail.com> wrote:
> > Yes, I know that, but as this issue is so good to write a perfect exploit
> > so I want to do more.
> >
> > I know the qemu planing and know this issue doesn't affect anyone.
> > I want to do a perfect work.
>
> The problem is that it does affect other people, because if
> Michael hadn't happened to also notice this bug then we would
> have released 3.1 next Tuesday with the bug still present.
> As it is we'll need to do an extra rc5, which we could have
> avoided if we'd known about this bug earlier, on Monday or Tuesday.
>

OK, next time I will report it directly like what I did before.

Thanks,
Li Qiang


>
> thanks
> -- PMM
>

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 11:12, Li Qiang <liq3ea@gmail.com> wrote:
> OK, next time I will report it directly like what I did before.

Thank you -- I appreciate that.

-- PMM

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

[Igor Mammedov]

On Thu,  6 Dec 2018 14:18:16 +0530
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.
> 
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Peter,
is it possible for fix to make into 3.1?

> ---
>  hw/i2c/pm_smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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;

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 09:48, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu,  6 Dec 2018 14:18:16 +0530
> 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.
> >
> > Reported-by: Michael Hanselmann <public@hansmi.ch>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
> Peter,
> is it possible for fix to make into 3.1?

This question would have been much better asked on Tuesday...

thanks
-- PMM

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

[Igor Mammedov]

On Thu,  6 Dec 2018 14:18:16 +0530
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.
> 
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
pls squash in:

Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability

> ---
>  hw/i2c/pm_smbus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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;

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

[P J P]

+-- On Thu, 6 Dec 2018, Igor Mammedov 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.
| > 
| > Reported-by: Michael Hanselmann <public@hansmi.ch>
| > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| pls squash in:
| 
| Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability

Do we need patch v2, or it can be done while merging it?

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

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 11:10, P J P <ppandit@redhat.com> wrote:
>
> +-- On Thu, 6 Dec 2018, Igor Mammedov 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.
> | >
> | > Reported-by: Michael Hanselmann <public@hansmi.ch>
> | > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> | pls squash in:
> |
> | Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability
>
> Do we need patch v2, or it can be done while merging it?

I can add in the Fixes line when I apply the patch to master.

It would be good if we could get some Reviewed-by: tags as well.

I think my current view is that we should apply this today,
and tag rc5 this afternoon (or maybe tomorrow?), and do the
final release on schedule on Tuesday.

Is there anything else in the pipeline that should go in rc5?

thanks
-- PMM

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

[Peter Maydell]

On Thu, 6 Dec 2018 at 11:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 6 Dec 2018 at 11:10, P J P <ppandit@redhat.com> wrote:
> >
> > +-- On Thu, 6 Dec 2018, Igor Mammedov 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.
> > | >
> > | > Reported-by: Michael Hanselmann <public@hansmi.ch>
> > | > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > | pls squash in:
> > |
> > | Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability
> >
> > Do we need patch v2, or it can be done while merging it?
>
> I can add in the Fixes line when I apply the patch to master.

Oh, I think we should also add to the commit message something
along the lines of:

"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 version."

to clarify that this is a serious bug but also that it's
not one that will be affecting anybody's production systems.

thanks
-- PMM

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

[P J P]

+-- On Thu, 6 Dec 2018, Peter Maydell wrote --+
| > > Do we need patch v2, or it can be done while merging it?
| >
| > I can add in the Fixes line when I apply the patch to master.
| 
| Oh, I think we should also add to the commit message something
| along the lines of:
| 
| "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 version."
| 
| to clarify that this is a serious bug but also that it's
| not one that will be affecting anybody's production systems.

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

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

[P J P]

+-- On Thu, 6 Dec 2018, P J P wrote --+
| | to clarify that this is a serious bug but also that it's
| | not one that will be affecting anybody's production systems.
| 
| Okay, preparing patch v2...

Sent revised patch
  [PATCH v1] i2c: pm_smbus: check smb_index before block transfer write

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

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

[li qiang]

在 2018/12/6 16:48, P J P 写道:
> 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.
>
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>   hw/i2c/pm_smbus.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> 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;

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

[Michael Hanselmann]

On 06.12.18 09:48, P J P wrote:
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Michael Hanselmann <public@hansmi.ch>

Best regards,
Michael

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

[Michael Hanselmann]

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

On 06.12.18 09:48, P J P wrote:
> 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.
> 
> Reported-by: Michael Hanselmann <public@hansmi.ch>

Considering that Li Qiang had already published his exploit for a couple
of hours (at the time of writing the URL is returning an HTTP 404 though
I'd seen it earlier) and with the patch being public I decided to also
publish my report:

https://hansmi.ch/articles/2018-12-qemu-pm-smbus-oob

I'd like to thank Prasad and his colleagues at Red Hat for the quick
response to my report (patch committed within less than 18 hours).

Best regards,
Michael

-- 
https://hansmi.ch/


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