i2c: pm_smbus: check smb_index before block transfer write

PATCH In review P J P 6 participants 21 messages Dec 6, 2018

lore

Share & tools

lore

P J P

author

2018-12-06 08:50 UTC #0 lore raw

PATCH 1/1 i2c: pm_smbus: check smb_index before block transfer write

From: Prasad J Pandit <[email protected]>

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 <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/i2c/pm_smbus.c | 3 +++
 1 file changed, 3 insertions(+)

hw/i2c/pm_smbus.c +3 -0

index 685a2378ed..03062740cc 100644
@@ -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;

Signature · 1 line

2.19.2

li qiang

2018-12-06 09:09 UTC #1 lore raw

Quoted 17 lines · P J P

From: Prasad J Pandit <[email protected]>
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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
  hw/i2c/pm_smbus.c | 3 +++
  1 file changed, 3 insertions(+)
hw/i2c/pm_smbus.c +0 -0
index 685a2378ed..03062740cc 100644

hw/i2c/pm_smbus.c static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val, · line 248

Quoted 9 lines

hw/i2c/pm_smbus.c +3 -0

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

li qiang

2018-12-06 10:14 UTC #2 lore raw

FYI:

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

Quoted 54 lines · li qiang

Quoted 17 lines · P J P
From: Prasad J Pandit <[email protected]>
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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
   hw/i2c/pm_smbus.c | 3 +++
   1 file changed, 3 insertions(+)
hw/i2c/pm_smbus.c +0 -0
index 685a2378ed..03062740cc 100644
hw/i2c/pm_smbus.c static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val, · line 248
Quoted 9 lines
hw/i2c/pm_smbus.c +3 -0
@@ -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

Peter Maydell

2018-12-06 10:16 UTC #3 lore raw

In reply to li qiang

Quoted 3 lines · li qiang

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

li qiang

2018-12-06 10:39 UTC #4 lore raw

Quoted 7 lines · Peter Maydell

Quoted 3 lines · li qiang
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

Quoted 2 lines

thanks
-- PMM

Peter Maydell

2018-12-06 10:46 UTC #5 lore raw

Quoted 11 lines · li qiang

Quoted 7 lines · Peter Maydell
Quoted 3 lines · li qiang
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...

Quoted 5 lines

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

Li Qiang

2018-12-06 11:00 UTC #6 lore raw

Quoted 12 lines · Peter Maydell <[email protected]> 于2018年12月6日周四下午6:46写道：

Quoted 11 lines · li qiang
Quoted 7 lines · Peter Maydell
Quoted 3 lines · li qiang
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...

Quoted 7 lines

Quoted 5 lines
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.

Quoted 4 lines

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

Quoted 4 lines

thanks
-- PMM

Peter Maydell

2018-12-06 11:06 UTC #7 lore raw

Quoted 5 lines · Li Qiang

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

Li Qiang

2018-12-06 11:13 UTC #8 lore raw

Quoted 4 lines · Peter Maydell <[email protected]> 于2018年12月6日周四下午7:05写道：

Quoted 3 lines · Li Qiang
Yes, I know that, but as this issue is so good to write a perfect exploit so I want to do more.

Quoted 9 lines

Quoted 2 lines
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

Quoted 4 lines

thanks
-- PMM

Peter Maydell

2018-12-06 11:14 UTC #9 lore raw

Quoted 1 line · Li Qiang

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

Thank you -- I appreciate that.

-- PMM

Igor Mammedov

Reviewed-by on v1 1/1

2018-12-06 09:48 UTC #10 lore raw

In reply to P J P

Quoted 8 lines · On Thu, 6 Dec 2018 14:18:16 +0530 P J P <[email protected]> wrote:

From: Prasad J Pandit <[email protected]>
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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

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

Quoted 18 lines

---
 hw/i2c/pm_smbus.c | 3 +++
 1 file changed, 3 insertions(+)

hw/i2c/pm_smbus.c +3 -0

index 685a2378ed..03062740cc 100644
@@ -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;

Peter Maydell

2018-12-06 10:14 UTC #11 lore raw

Quoted 10 lines · Igor Mammedov

Quoted 6 lines · On Thu, 6 Dec 2018 14:18:16 +0530 P J P <[email protected]> wrote:
From: Prasad J Pandit <[email protected]>
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.

Quoted 6 lines

Quoted 2 lines
Reported-by Michael Hanselmann <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>
Peter,
is it possible for fix to make into 3.1?

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

thanks
-- PMM

Igor Mammedov

2018-12-06 09:58 UTC #12 lore raw

In reply to P J P

Quoted 8 lines · On Thu, 6 Dec 2018 14:18:16 +0530 P J P <[email protected]> wrote:

From: Prasad J Pandit <[email protected]>
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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>

pls squash in:

Fixes 38ad4fae43 i2c: pm_smbus: Add block transfer capability

Quoted 18 lines

---
 hw/i2c/pm_smbus.c | 3 +++
 1 file changed, 3 insertions(+)

hw/i2c/pm_smbus.c +3 -0

index 685a2378ed..03062740cc 100644
@@ -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;

P J P

author

2018-12-06 11:08 UTC #13 lore raw

+-- On Thu, 6 Dec 2018, Igor Mammedov wrote --+
| > From: Prasad J Pandit <[email protected]>
| > 
| > 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 <[email protected]>
| > Signed-off-by: Prasad J Pandit <[email protected]>
| 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

Peter Maydell

2018-12-06 11:19 UTC #14 lore raw

Quoted 14 lines · P J P

+-- On Thu, 6 Dec 2018, Igor Mammedov wrote --+
| > From: Prasad J Pandit <[email protected]>
| >
| > 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 <[email protected]>
| > Signed-off-by: Prasad J Pandit <[email protected]>
| pls squash in:
|
| Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability

Quoted 1 line

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

Peter Maydell

2018-12-06 11:22 UTC #15 lore raw

Quoted 16 lines · Peter Maydell

Quoted 14 lines · P J P

+-- On Thu, 6 Dec 2018, Igor Mammedov wrote --+
| > From: Prasad J Pandit <[email protected]>
| >
| > 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 <[email protected]>
| > Signed-off-by: Prasad J Pandit <[email protected]>
| pls squash in:
|
| Fixes: 38ad4fae43 i2c: pm_smbus: Add block transfer capability

Quoted 3 lines

Quoted 1 line
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

P J P

author

2018-12-06 12:04 UTC #16 lore raw

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

P J P

author

2018-12-06 12:22 UTC #17 lore raw

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

li qiang

Reviewed-by on v1 1/1

2018-12-06 11:33 UTC #18 lore raw

In reply to P J P

Quoted 8 lines · P J P

From: Prasad J Pandit <[email protected]>
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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>

Reviewed-by Li Qiang <[email protected]>

Quoted 18 lines

---
  hw/i2c/pm_smbus.c | 3 +++
  1 file changed, 3 insertions(+)

hw/i2c/pm_smbus.c +3 -0

index 685a2378ed..03062740cc 100644
@@ -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;

Michael Hanselmann

Reviewed-by on v1 1/1

2018-12-06 11:35 UTC #19 lore raw

In reply to P J P

Quoted 2 lines · P J P

Reported-by Michael Hanselmann <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>

Reviewed-by Michael Hanselmann <[email protected]>

Best regards,
Michael

Michael Hanselmann

2018-12-06 20:16 UTC #20 lore raw

In reply to P J P

Quoted 5 lines · P J P

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 <[email protected]>

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

Signature · 1 line

https://hansmi.ch/