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-sta...@nongnu.org
> >> Reported-by: Qiuhao Li <qiuhao...@outlook.com>
> >> Fixes: f432fdfa121 ("support changed namespace asynchronous event")
> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@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! :)
signature.asc
Description: PGP signature
