* [PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
@ 2021-11-17 12:35 Philippe Mathieu-Daudé
2021-11-17 12:35 ` [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947) Philippe Mathieu-Daudé
2021-11-17 12:35 ` [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info() Philippe Mathieu-Daudé
0 siblings, 2 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-17 12:35 UTC (permalink / raw)
To: qemu-devel
Cc: Gaurav Kamathe, qemu-block, Qiuhao Li, Klaus Jensen, Keith Busch,
Philippe Mathieu-Daudé
Since v1:
- Do not add more buffer overflows in modify nvme_smart_info(),
nvme_fw_log_info() and nvme_cmd_effects() (Klaus)
- Split nvme_error_info() change in another patch
Philippe Mathieu-Daudé (2):
hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist
(CVE-2021-3947)
hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()
hw/nvme/ctrl.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)
2021-11-17 12:35 [PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947) Philippe Mathieu-Daudé
@ 2021-11-17 12:35 ` Philippe Mathieu-Daudé
2021-11-17 13:08 ` Klaus Jensen
2021-11-17 12:35 ` [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info() Philippe Mathieu-Daudé
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-17 12:35 UTC (permalink / raw)
To: qemu-devel
Cc: Gaurav Kamathe, qemu-block, Qiuhao Li, qemu-stable, 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 | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfa..93a24464647 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -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) {
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()
2021-11-17 12:35 [PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947) Philippe Mathieu-Daudé
2021-11-17 12:35 ` [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947) Philippe Mathieu-Daudé
@ 2021-11-17 12:35 ` Philippe Mathieu-Daudé
2021-11-17 13:10 ` Klaus Jensen
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-17 12:35 UTC (permalink / raw)
To: qemu-devel
Cc: Gaurav Kamathe, qemu-block, Qiuhao Li, qemu-stable, 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
Fixes: 94a7897c41d ("add support for the get log page command")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/nvme/ctrl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 93a24464647..7414f3b4dd1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -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(errlog)) {
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);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)
2021-11-17 12:35 ` [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947) Philippe Mathieu-Daudé
@ 2021-11-17 13:08 ` Klaus Jensen
0 siblings, 0 replies; 5+ messages in thread
From: Klaus Jensen @ 2021-11-17 13:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Gaurav Kamathe, Qiuhao Li, qemu-block, qemu-stable, qemu-devel,
Keith Busch
[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]
On Nov 17 13:35, 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 | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 6a571d18cfa..93a24464647 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -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) {
The issue I mentioned with off > sizeof(nslist). I'll send a fix that
just deals with it like all the other log pages.
There is probably a better way to do these checks, but I'm not sure how
right now.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()
2021-11-17 12:35 ` [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info() Philippe Mathieu-Daudé
@ 2021-11-17 13:10 ` Klaus Jensen
0 siblings, 0 replies; 5+ messages in thread
From: Klaus Jensen @ 2021-11-17 13:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Gaurav Kamathe, Qiuhao Li, qemu-block, qemu-stable, qemu-devel,
Keith Busch
[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]
On Nov 17 13:35, 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
> Fixes: 94a7897c41d ("add support for the get log page command")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/nvme/ctrl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 93a24464647..7414f3b4dd1 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -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(errlog)) {
> 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);
> }
> --
> 2.31.1
>
I don't see any buffer overrun issue prior to this patch. However, there
is a functional bug since the offset is not added in the c2h.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-17 13:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 12:35 [PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947) Philippe Mathieu-Daudé
2021-11-17 12:35 ` [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947) Philippe Mathieu-Daudé
2021-11-17 13:08 ` Klaus Jensen
2021-11-17 12:35 ` [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info() Philippe Mathieu-Daudé
2021-11-17 13:10 ` Klaus Jensen
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.