* [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
@ 2021-11-11 15:31 Philippe Mathieu-Daudé
2021-11-11 18:08 ` Klaus Jensen
0 siblings, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-11 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Mauro Matteo Cascella, qemu-block, qemu-stable, Qiuhao Li,
Klaus Jensen, Keith Busch, Philippe Mathieu-Daudé
Both 'buf_len' and 'off' arguments are under guest control.
Since nvme_c2h() doesn't check out of boundary access, the
caller must check for eventual buffer overrun on 'trans_len'.
Cc: qemu-stable@nongnu.org
Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
Fixes: f432fdfa121 ("support changed namespace asynchronous event")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/nvme/ctrl.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfa..634b290e069 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
NvmeNamespace *ns;
time_t current_ms;
- if (off >= sizeof(smart)) {
+ trans_len = MIN(sizeof(smart) - off, buf_len);
+ if (trans_len >= sizeof(smart)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
}
}
- trans_len = MIN(sizeof(smart) - off, buf_len);
smart.critical_warning = n->smart_critical_warning;
smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
@@ -4130,12 +4130,12 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n, uint32_t buf_len, uint64_t off,
.afi = 0x1,
};
- if (off >= sizeof(fw_log)) {
+ trans_len = MIN(sizeof(fw_log) - off, buf_len);
+ if (trans_len >= sizeof(fw_log)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
- trans_len = MIN(sizeof(fw_log) - off, buf_len);
return nvme_c2h(n, (uint8_t *) &fw_log + off, trans_len, req);
}
@@ -4146,7 +4146,8 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
uint32_t trans_len;
NvmeErrorLog errlog;
- if (off >= sizeof(errlog)) {
+ trans_len = MIN(sizeof(errlog) - off, buf_len);
+ if (trans_len >= sizeof(trans_len)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -4155,7 +4156,6 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
}
memset(&errlog, 0x0, sizeof(errlog));
- trans_len = MIN(sizeof(errlog) - off, buf_len);
return nvme_c2h(n, (uint8_t *)&errlog, trans_len, req);
}
@@ -4168,8 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
int i = 0;
uint32_t nsid;
- memset(nslist, 0x0, sizeof(nslist));
trans_len = MIN(sizeof(nslist) - off, buf_len);
+ if (trans_len >= sizeof(nslist)) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+ memset(nslist, 0x0, sizeof(nslist));
while ((nsid = find_first_bit(n->changed_nsids, NVME_CHANGED_NSID_SIZE)) !=
NVME_CHANGED_NSID_SIZE) {
@@ -4209,7 +4212,8 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
const uint32_t *src_iocs = NULL;
uint32_t trans_len;
- if (off >= sizeof(log)) {
+ trans_len = MIN(sizeof(log) - off, buf_len);
+ if (trans_len >= sizeof(log)) {
trace_pci_nvme_err_invalid_log_page_offset(off, sizeof(log));
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -4237,8 +4241,6 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
memcpy(log.iocs, src_iocs, sizeof(log.iocs));
}
- trans_len = MIN(sizeof(log) - off, buf_len);
-
return nvme_c2h(n, ((uint8_t *)&log) + off, trans_len, req);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
2021-11-11 15:31 [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947) Philippe Mathieu-Daudé
@ 2021-11-11 18:08 ` Klaus Jensen
2021-11-11 18:46 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 4+ messages in thread
From: Klaus Jensen @ 2021-11-11 18:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Mauro Matteo Cascella, qemu-block, qemu-stable, Qiuhao Li,
qemu-devel, Keith Busch
[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]
On Nov 11 16:31, Philippe Mathieu-Daudé wrote:
> Both 'buf_len' and 'off' arguments are under guest control.
> Since nvme_c2h() doesn't check out of boundary access, the
> caller must check for eventual buffer overrun on 'trans_len'.
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
> Fixes: f432fdfa121 ("support changed namespace asynchronous event")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/nvme/ctrl.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 6a571d18cfa..634b290e069 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> NvmeNamespace *ns;
> time_t current_ms;
>
> - if (off >= sizeof(smart)) {
> + trans_len = MIN(sizeof(smart) - off, buf_len);
> + if (trans_len >= sizeof(smart)) {
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> @@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> }
> }
>
> - trans_len = MIN(sizeof(smart) - off, buf_len);
> smart.critical_warning = n->smart_critical_warning;
>
> smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
Uhm. Hehe.
This "fix" breaks all log pages. Take smart_info as an example. Say the
offset is zero and the buffer length is 512. The transfer length
(trans_len) then becomes 512 and it ends up returning Invalid Field in
Command because trans_len equals sizeof(smart).
Worse, this "fix" actually *introduce* oob's all over the place if
off > sizeof(smart) && buf_len < sizeof(smart)
Example
sizeof(smart) = 512
off = 516 (must be dword aligned to get to this spot)
buf_len = 4 (same, but is always aligned)
=> trans_len = min(512 - 516, 4) = 4
if (1 >= 512) => false
And we end up with
nvme_c2h(n, &smart + 516, 4, req);
I suggest that we only fix nvme_changed_nslist ;)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
2021-11-11 18:08 ` Klaus Jensen
@ 2021-11-11 18:46 ` Philippe Mathieu-Daudé
2021-11-11 20:42 ` Klaus Jensen
0 siblings, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-11 18:46 UTC (permalink / raw)
To: Klaus Jensen
Cc: Mauro Matteo Cascella, qemu-block, qemu-stable, Qiuhao Li,
qemu-devel, Keith Busch
On 11/11/21 19:08, Klaus Jensen wrote:
> On Nov 11 16:31, Philippe Mathieu-Daudé wrote:
>> Both 'buf_len' and 'off' arguments are under guest control.
>> Since nvme_c2h() doesn't check out of boundary access, the
>> caller must check for eventual buffer overrun on 'trans_len'.
>>
>> Cc: qemu-stable@nongnu.org
>> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
>> Fixes: f432fdfa121 ("support changed namespace asynchronous event")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> hw/nvme/ctrl.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> index 6a571d18cfa..634b290e069 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>> NvmeNamespace *ns;
>> time_t current_ms;
>>
>> - if (off >= sizeof(smart)) {
>> + trans_len = MIN(sizeof(smart) - off, buf_len);
>> + if (trans_len >= sizeof(smart)) {
>> return NVME_INVALID_FIELD | NVME_DNR;
>> }
>>
>> @@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>> }
>> }
>>
>> - trans_len = MIN(sizeof(smart) - off, buf_len);
>> smart.critical_warning = n->smart_critical_warning;
>>
>> smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
>
> Uhm. Hehe.
>
> This "fix" breaks all log pages. Take smart_info as an example. Say the
> offset is zero and the buffer length is 512. The transfer length
> (trans_len) then becomes 512 and it ends up returning Invalid Field in
> Command because trans_len equals sizeof(smart).
>
> Worse, this "fix" actually *introduce* oob's all over the place if
>
> off > sizeof(smart) && buf_len < sizeof(smart)
>
>
> Example
>
> sizeof(smart) = 512
> off = 516 (must be dword aligned to get to this spot)
> buf_len = 4 (same, but is always aligned)
> => trans_len = min(512 - 516, 4) = 4
> if (1 >= 512) => false
>
> And we end up with
>
> nvme_c2h(n, &smart + 516, 4, req);
Doh, I missed the '+ off' part in the nvme_c2h() call...
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
2021-11-11 18:46 ` Philippe Mathieu-Daudé
@ 2021-11-11 20:42 ` Klaus Jensen
0 siblings, 0 replies; 4+ messages in thread
From: Klaus Jensen @ 2021-11-11 20:42 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Qiuhao Li, Mauro Matteo Cascella, qemu-block, qemu-devel,
qemu-stable, Keith Busch
[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]
On Nov 11 19:46, Philippe Mathieu-Daudé wrote:
> On 11/11/21 19:08, Klaus Jensen wrote:
> > On Nov 11 16:31, Philippe Mathieu-Daudé wrote:
> >> Both 'buf_len' and 'off' arguments are under guest control.
> >> Since nvme_c2h() doesn't check out of boundary access, the
> >> caller must check for eventual buffer overrun on 'trans_len'.
> >>
> >> Cc: qemu-stable@nongnu.org
> >> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
> >> Fixes: f432fdfa121 ("support changed namespace asynchronous event")
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> hw/nvme/ctrl.c | 22 ++++++++++++----------
> >> 1 file changed, 12 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> >> index 6a571d18cfa..634b290e069 100644
> >> --- a/hw/nvme/ctrl.c
> >> +++ b/hw/nvme/ctrl.c
> >> @@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> >> NvmeNamespace *ns;
> >> time_t current_ms;
> >>
> >> - if (off >= sizeof(smart)) {
> >> + trans_len = MIN(sizeof(smart) - off, buf_len);
> >> + if (trans_len >= sizeof(smart)) {
> >> return NVME_INVALID_FIELD | NVME_DNR;
> >> }
> >>
> >> @@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> >> }
> >> }
> >>
> >> - trans_len = MIN(sizeof(smart) - off, buf_len);
> >> smart.critical_warning = n->smart_critical_warning;
> >>
> >> smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
> >
> > Uhm. Hehe.
> >
> > This "fix" breaks all log pages. Take smart_info as an example. Say the
> > offset is zero and the buffer length is 512. The transfer length
> > (trans_len) then becomes 512 and it ends up returning Invalid Field in
> > Command because trans_len equals sizeof(smart).
> >
> > Worse, this "fix" actually *introduce* oob's all over the place if
> >
> > off > sizeof(smart) && buf_len < sizeof(smart)
> >
> >
> > Example
> >
> > sizeof(smart) = 512
> > off = 516 (must be dword aligned to get to this spot)
> > buf_len = 4 (same, but is always aligned)
> > => trans_len = min(512 - 516, 4) = 4
> > if (1 >= 512) => false
> >
> > And we end up with
> >
> > nvme_c2h(n, &smart + 516, 4, req);
>
> Doh, I missed the '+ off' part in the nvme_c2h() call...
>
You wanna send a v2 just adding the check on offset like the other log
pages, or was there a particular reason you think it could use a
refactor?
I'm open to whatever can make it safer! :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-11 20:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 15:31 [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947) Philippe Mathieu-Daudé
2021-11-11 18:08 ` Klaus Jensen
2021-11-11 18:46 ` Philippe Mathieu-Daudé
2021-11-11 20:42 ` Klaus Jensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).