* [Qemu-devel] [PATCH] lsi53c895a: check message length value
@ 2018-10-25 20:09 P J P
2018-10-25 20:40 ` Ameya More
2018-10-25 23:18 ` Paolo Bonzini
0 siblings, 2 replies; 9+ messages in thread
From: P J P @ 2018-10-25 20:09 UTC (permalink / raw)
To: Qemu Developers; +Cc: Ameya More, Paolo Bonzini, Fam Zheng, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
While writing a message in 'lsi_do_msgin', message length value
in msg_len could be invalid, add check to avoid OOB access issue.
Reported-by: Ameya More <ameya.more@oracle.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/scsi/lsi53c895a.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index d1e6534311..a266c5a113 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -205,7 +205,7 @@ typedef struct {
/* Action to take at the end of a MSG IN phase.
0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN. */
int msg_action;
- int msg_len;
+ uint8_t msg_len;
uint8_t msg[LSI_MAX_MSGIN_LEN];
/* 0 if SCRIPTS are running or stopped.
* 1 if a Wait Reselect instruction has been issued.
@@ -861,12 +861,15 @@ static void lsi_do_status(LSIState *s)
static void lsi_do_msgin(LSIState *s)
{
- int len;
+ uint8_t len;
trace_lsi_do_msgin(s->dbc, s->msg_len);
s->sfbr = s->msg[0];
len = s->msg_len;
if (len > s->dbc)
len = s->dbc;
+ if (len > LSI_MAX_MSGIN_LEN) {
+ len = LSI_MAX_MSGIN_LEN;
+ }
pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
/* Linux drivers rely on the last byte being in the SIDL. */
s->sidl = s->msg[len - 1];
@@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
VMSTATE_INT32(carry, LSIState),
VMSTATE_INT32(status, LSIState),
VMSTATE_INT32(msg_action, LSIState),
- VMSTATE_INT32(msg_len, LSIState),
+ VMSTATE_UINT8(msg_len, LSIState),
VMSTATE_BUFFER(msg, LSIState),
VMSTATE_INT32(waiting, LSIState),
--
2.17.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-25 20:09 [Qemu-devel] [PATCH] lsi53c895a: check message length value P J P
@ 2018-10-25 20:40 ` Ameya More
2018-10-26 9:25 ` P J P
2018-10-25 23:18 ` Paolo Bonzini
1 sibling, 1 reply; 9+ messages in thread
From: Ameya More @ 2018-10-25 20:40 UTC (permalink / raw)
To: P J P, Qemu Developers
Cc: Paolo Bonzini, Fam Zheng, Prasad J Pandit, Mark Kanda
Hi Prasad,
Thanks for following up on this. While Mark and I reported this issue to
you, it was actually discovered by Dejvau Security and they should
receive credit for reporting this issue.
http://www.dejavusecurity.com
Thanks,
Ameya
On 10/25/2018 03:09 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While writing a message in 'lsi_do_msgin', message length value
> in msg_len could be invalid, add check to avoid OOB access issue.
>
> Reported-by: Ameya More <ameya.more@oracle.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/scsi/lsi53c895a.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534311..a266c5a113 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -205,7 +205,7 @@ typedef struct {
> /* Action to take at the end of a MSG IN phase.
> 0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN. */
> int msg_action;
> - int msg_len;
> + uint8_t msg_len;
> uint8_t msg[LSI_MAX_MSGIN_LEN];
> /* 0 if SCRIPTS are running or stopped.
> * 1 if a Wait Reselect instruction has been issued.
> @@ -861,12 +861,15 @@ static void lsi_do_status(LSIState *s)
>
> static void lsi_do_msgin(LSIState *s)
> {
> - int len;
> + uint8_t len;
> trace_lsi_do_msgin(s->dbc, s->msg_len);
> s->sfbr = s->msg[0];
> len = s->msg_len;
> if (len > s->dbc)
> len = s->dbc;
> + if (len > LSI_MAX_MSGIN_LEN) {
> + len = LSI_MAX_MSGIN_LEN;
> + }
> pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
> /* Linux drivers rely on the last byte being in the SIDL. */
> s->sidl = s->msg[len - 1];
> @@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
> VMSTATE_INT32(carry, LSIState),
> VMSTATE_INT32(status, LSIState),
> VMSTATE_INT32(msg_action, LSIState),
> - VMSTATE_INT32(msg_len, LSIState),
> + VMSTATE_UINT8(msg_len, LSIState),
> VMSTATE_BUFFER(msg, LSIState),
> VMSTATE_INT32(waiting, LSIState),
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-25 20:09 [Qemu-devel] [PATCH] lsi53c895a: check message length value P J P
2018-10-25 20:40 ` Ameya More
@ 2018-10-25 23:18 ` Paolo Bonzini
2018-10-26 9:36 ` P J P
1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2018-10-25 23:18 UTC (permalink / raw)
To: P J P, Qemu Developers; +Cc: Ameya More, Fam Zheng, Prasad J Pandit
On 25/10/2018 22:09, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While writing a message in 'lsi_do_msgin', message length value
> in msg_len could be invalid, add check to avoid OOB access issue.
>
> Reported-by: Ameya More <ameya.more@oracle.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/scsi/lsi53c895a.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534311..a266c5a113 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -205,7 +205,7 @@ typedef struct {
> /* Action to take at the end of a MSG IN phase.
> 0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN. */
> int msg_action;
> - int msg_len;
> + uint8_t msg_len;
Not wrong per se, but it's also not clear why it's needed. I understand
that you want to switch from signed to unsigned, but it is not mentioned
in the commit message.
The switch to 8-bit, and below from VMSTATE_INT32 to VMSTATE_UINT8, is
wrong because it changes the format of the live migration stream.
> uint8_t msg[LSI_MAX_MSGIN_LEN];
> /* 0 if SCRIPTS are running or stopped.
> * 1 if a Wait Reselect instruction has been issued.
> @@ -861,12 +861,15 @@ static void lsi_do_status(LSIState *s)
>
> static void lsi_do_msgin(LSIState *s)
> {
> - int len;
> + uint8_t len;
> trace_lsi_do_msgin(s->dbc, s->msg_len);
> s->sfbr = s->msg[0];
> len = s->msg_len;
> if (len > s->dbc)
> len = s->dbc;
> + if (len > LSI_MAX_MSGIN_LEN) {
> + len = LSI_MAX_MSGIN_LEN;
> + }
I'm not sure it's appropriate to check for out of bounds reads here,
because if s->msg_len is greater than LSI_MAX_MSGIN_LEN, then this test
doesn't exclude you've already had an out of bounds write before.
Indeed the msg_len is checked in lsi_add_msg_byte in order to avoid out
of bounds accesses in either lsi_add_msg_byte or lsi_do_msgin. You
could assert here that the variable is in range, I guess.
However, the out of bounds s->msg_len can actually happen in one other
case: namely, if a malicious live migration stream includes a bogus
s->msg_len. Such live migration stream should be rejected; the fix for
that is to add a lsi_post_load function, point to it in
vmstate_lsi_scsi, and check there for s->msg_len <= LSI_MAX_MSGIN_LEN.
Thanks,
Paolo
> pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
> /* Linux drivers rely on the last byte being in the SIDL. */
> s->sidl = s->msg[len - 1];
> @@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
> VMSTATE_INT32(carry, LSIState),
> VMSTATE_INT32(status, LSIState),
> VMSTATE_INT32(msg_action, LSIState),
> - VMSTATE_INT32(msg_len, LSIState),
> + VMSTATE_UINT8(msg_len, LSIState),
> VMSTATE_BUFFER(msg, LSIState),
> VMSTATE_INT32(waiting, LSIState),
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-25 20:40 ` Ameya More
@ 2018-10-26 9:25 ` P J P
2018-10-26 14:01 ` Mark Kanda
0 siblings, 1 reply; 9+ messages in thread
From: P J P @ 2018-10-26 9:25 UTC (permalink / raw)
To: Ameya More; +Cc: Qemu Developers, Paolo Bonzini, Fam Zheng, Mark Kanda
+-- On Thu, 25 Oct 2018, Ameya More wrote --+
| While Mark and I reported this issue to you, it was actually discovered by
| Dejvau Security and they should receive credit for reporting this issue.
| http://www.dejavusecurity.com
I see; Would it be possible to share email-id of the original reporter to
include in the commit log message?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-25 23:18 ` Paolo Bonzini
@ 2018-10-26 9:36 ` P J P
0 siblings, 0 replies; 9+ messages in thread
From: P J P @ 2018-10-26 9:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Qemu Developers, Ameya More, Fam Zheng
+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| > - int msg_len;
| > + uint8_t msg_len;
|
| Not wrong per se, but it's also not clear why it's needed. I understand
| that you want to switch from signed to unsigned, but it is not mentioned
| in the commit message.
Changed to uint8_t because IIUC 'msg_len' is likely be < LSI_MAX_MSGIN_LEN=8.
And uint32_t seems rather large for it.
| The switch to 8-bit, and below from VMSTATE_INT32 to VMSTATE_UINT8, is
| wrong because it changes the format of the live migration stream.
I see.
| I'm not sure it's appropriate to check for out of bounds reads here,
| because if s->msg_len is greater than LSI_MAX_MSGIN_LEN, then this test
| doesn't exclude you've already had an out of bounds write before.
| Indeed the msg_len is checked in lsi_add_msg_byte in order to avoid out
| of bounds accesses in either lsi_add_msg_byte or lsi_do_msgin. You
| could assert here that the variable is in range, I guess.
Okay.
| However, the out of bounds s->msg_len can actually happen in one other
| case: namely, if a malicious live migration stream includes a bogus
| s->msg_len. Such live migration stream should be rejected; the fix for
| that is to add a lsi_post_load function, point to it in
| vmstate_lsi_scsi, and check there for s->msg_len <= LSI_MAX_MSGIN_LEN.
Okay, sending a revised patch v1 in a bit.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-26 9:25 ` P J P
@ 2018-10-26 14:01 ` Mark Kanda
2018-10-26 18:37 ` P J P
0 siblings, 1 reply; 9+ messages in thread
From: Mark Kanda @ 2018-10-26 14:01 UTC (permalink / raw)
To: P J P, Ameya More; +Cc: Qemu Developers, Paolo Bonzini, Fam Zheng
On 10/26/2018 4:25 AM, P J P wrote:
> +-- On Thu, 25 Oct 2018, Ameya More wrote --+
> | While Mark and I reported this issue to you, it was actually discovered by
> | Dejvau Security and they should receive credit for reporting this issue.
> | http://www.dejavusecurity.com
>
> I see; Would it be possible to share email-id of the original reporter to
> include in the commit log message?
Deja vu requested that we include the following text in the commit message:
Discovered by Deja vu Security. Reported by Oracle.
Would that be acceptable?
Thanks,
-Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-26 14:01 ` Mark Kanda
@ 2018-10-26 18:37 ` P J P
2018-10-26 18:45 ` Mark Kanda
0 siblings, 1 reply; 9+ messages in thread
From: P J P @ 2018-10-26 18:37 UTC (permalink / raw)
To: Mark Kanda; +Cc: Ameya More, Qemu Developers, Paolo Bonzini, Fam Zheng
+-- On Fri, 26 Oct 2018, Mark Kanda wrote --+
| Deja vu requested that we include the following text in the commit message:
|
| Discovered by Deja vu Security. Reported by Oracle.
|
| Would that be acceptable?
Generally an email-id is used/preferred in the commit log message. We could
use above for acknowledgement and avoid Reported-by in the commit log message
if that suits Deja vu team.
Please let me know your/their preference.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-26 18:37 ` P J P
@ 2018-10-26 18:45 ` Mark Kanda
2018-10-26 19:08 ` P J P
0 siblings, 1 reply; 9+ messages in thread
From: Mark Kanda @ 2018-10-26 18:45 UTC (permalink / raw)
To: P J P; +Cc: Ameya More, Qemu Developers, Paolo Bonzini, Fam Zheng
On 10/26/2018 1:37 PM, P J P wrote:
> +-- On Fri, 26 Oct 2018, Mark Kanda wrote --+
> | Deja vu requested that we include the following text in the commit message:
> |
> | Discovered by Deja vu Security. Reported by Oracle.
> |
> | Would that be acceptable?
>
> Generally an email-id is used/preferred in the commit log message. We could
> use above for acknowledgement and avoid Reported-by in the commit log message
> if that suits Deja vu team.
>
> Please let me know your/their preference.
>
Yes, please use that acknowledgement text in lieu of a 'Reported-by' line.
Thanks,
-Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] lsi53c895a: check message length value
2018-10-26 18:45 ` Mark Kanda
@ 2018-10-26 19:08 ` P J P
0 siblings, 0 replies; 9+ messages in thread
From: P J P @ 2018-10-26 19:08 UTC (permalink / raw)
To: Mark Kanda; +Cc: Ameya More, Qemu Developers, Paolo Bonzini, Fam Zheng
+-- On Fri, 26 Oct 2018, Mark Kanda wrote --+
| Yes, please use that acknowledgement text in lieu of a 'Reported-by' line.
Okay, thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-26 19:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 20:09 [Qemu-devel] [PATCH] lsi53c895a: check message length value P J P
2018-10-25 20:40 ` Ameya More
2018-10-26 9:25 ` P J P
2018-10-26 14:01 ` Mark Kanda
2018-10-26 18:37 ` P J P
2018-10-26 18:45 ` Mark Kanda
2018-10-26 19:08 ` P J P
2018-10-25 23:18 ` Paolo Bonzini
2018-10-26 9:36 ` P J P
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.