Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read
On Tue, 2020-03-31 at 14:48 +0200, Klaus Birkelund Jensen wrote:
> On Mar 31 13:41, Maxim Levitsky wrote:
> > On Tue, 2020-03-31 at 07:39 +0200, Klaus Birkelund Jensen wrote:
> > > On Mar 25 12:38, Maxim Levitsky wrote:
> > > > Note that this patch still contains a bug that it removes the check
> > > > against the accessed
> > > > size, which you fix in later patch.
> > > > I prefer to not add a bug in first place
> > > > However if you have a reason for this, I won't mind.
> > > >
> > >
> > > So yeah. The resons is that there is actually no bug at this point
> > > because the controller only supports PRPs. I actually thought there was
> > > a bug as well and reported it to qemu-security some months ago as a
> > > potential out of bounds access. I was then schooled by Keith on how PRPs
> > > work ;) Below is a paraphrased version of Keiths analysis.
> > >
> > > The PRPs does not cross page boundaries:
> >
> > True
> >
> > >
> > > trans_len = n->page_size - (prp1 % n->page_size);
> > >
> > > The PRPs are always verified to be page aligned:
> >
> > True
> > >
> > > if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
> > >
> > > and the transfer length wont go above page size. So, since the beginning
> > > of the address is within the CMB and considering that the CMB is of an
> > > MB aligned and sized granularity, then we can never cross outside it
> > > with PRPs.
> >
> > I understand now, however the reason I am arguing about this is
> > that this patch actually _removes_ the size bound check
> >
> > It was before the patch:
> >
> > n->cmbsz && addr >= n->ctrl_mem.addr &&
> > addr < (n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)
> >
>
> I don't think it does - the check is just moved to nvme_addr_is_cmb:
>
> static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> {
> hwaddr low = n->ctrl_mem.addr;
> hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
>
> return addr >= low && addr < hi;
> }
>
> We check that `addr` is less than `hi`. Maybe the name is unfortunate...
>
>
Oh, I am just blind! sorry about that.
You are 100% right.
Best regards,
Maxim Levitsky
Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read
On Mar 31 13:41, Maxim Levitsky wrote:
> On Tue, 2020-03-31 at 07:39 +0200, Klaus Birkelund Jensen wrote:
> > On Mar 25 12:38, Maxim Levitsky wrote:
> > > Note that this patch still contains a bug that it removes the check
> > > against the accessed
> > > size, which you fix in later patch.
> > > I prefer to not add a bug in first place
> > > However if you have a reason for this, I won't mind.
> > >
> >
> > So yeah. The resons is that there is actually no bug at this point
> > because the controller only supports PRPs. I actually thought there was
> > a bug as well and reported it to qemu-security some months ago as a
> > potential out of bounds access. I was then schooled by Keith on how PRPs
> > work ;) Below is a paraphrased version of Keiths analysis.
> >
> > The PRPs does not cross page boundaries:
> True
>
> >
> > trans_len = n->page_size - (prp1 % n->page_size);
> >
> > The PRPs are always verified to be page aligned:
> True
> >
> > if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
> >
> > and the transfer length wont go above page size. So, since the beginning
> > of the address is within the CMB and considering that the CMB is of an
> > MB aligned and sized granularity, then we can never cross outside it
> > with PRPs.
> I understand now, however the reason I am arguing about this is
> that this patch actually _removes_ the size bound check
>
> It was before the patch:
>
> n->cmbsz && addr >= n->ctrl_mem.addr &&
> addr < (n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)
>
I don't think it does - the check is just moved to nvme_addr_is_cmb:
static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
{
hwaddr low = n->ctrl_mem.addr;
hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
return addr >= low && addr < hi;
}
We check that `addr` is less than `hi`. Maybe the name is unfortunate...
Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read
On Tue, 2020-03-31 at 07:39 +0200, Klaus Birkelund Jensen wrote:
> On Mar 25 12:38, Maxim Levitsky wrote:
> > On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > > From: Klaus Jensen
> > >
> > > Pull the controller memory buffer check to its own function. The check
> > > will be used on its own in later patches.
> > >
> > > Signed-off-by: Klaus Jensen
> > > Acked-by: Keith Busch
> > > ---
> > > hw/block/nvme.c | 16
> > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index b38d7e548a60..08a83d449de3 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -52,14 +52,22 @@
> > >
> > > static void nvme_process_sq(void *opaque);
> > >
> > > +static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> > > +{
> > > +hwaddr low = n->ctrl_mem.addr;
> > > +hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
> > > +
> > > +return addr >= low && addr < hi;
> > > +}
> > > +
> > > static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> > > {
> > > -if (n->cmbsz && addr >= n->ctrl_mem.addr &&
> > > -addr < (n->ctrl_mem.addr +
> > > int128_get64(n->ctrl_mem.size))) {
> > > +if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
> > > memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
> > > -} else {
> > > -pci_dma_read(>parent_obj, addr, buf, size);
> > > +return;
> > > }
> > > +
> > > +pci_dma_read(>parent_obj, addr, buf, size);
> > > }
> > >
> > > static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
> >
> > Note that this patch still contains a bug that it removes the check against
> > the accessed
> > size, which you fix in later patch.
> > I prefer to not add a bug in first place
> > However if you have a reason for this, I won't mind.
> >
>
> So yeah. The resons is that there is actually no bug at this point
> because the controller only supports PRPs. I actually thought there was
> a bug as well and reported it to qemu-security some months ago as a
> potential out of bounds access. I was then schooled by Keith on how PRPs
> work ;) Below is a paraphrased version of Keiths analysis.
>
> The PRPs does not cross page boundaries:
True
>
> trans_len = n->page_size - (prp1 % n->page_size);
>
> The PRPs are always verified to be page aligned:
True
>
> if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
>
> and the transfer length wont go above page size. So, since the beginning
> of the address is within the CMB and considering that the CMB is of an
> MB aligned and sized granularity, then we can never cross outside it
> with PRPs.
I understand now, however the reason I am arguing about this is
that this patch actually _removes_ the size bound check
It was before the patch:
n->cmbsz && addr >= n->ctrl_mem.addr &&
addr < (n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)
>
> I could add the check at this point (because it *is* needed for when
> SGLs are introduced), but I think it would just be noise and I would
> need to explain why the check is there, but not really needed at this
> point. Instead I'm adding a new patch before the SGL patch that explains
> this.
Best regards,
Maxim Levitsky
Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read
On Mar 25 12:38, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Pull the controller memory buffer check to its own function. The check
> > will be used on its own in later patches.
> >
> > Signed-off-by: Klaus Jensen
> > Acked-by: Keith Busch
> > ---
> > hw/block/nvme.c | 16
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index b38d7e548a60..08a83d449de3 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -52,14 +52,22 @@
> >
> > static void nvme_process_sq(void *opaque);
> >
> > +static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> > +{
> > +hwaddr low = n->ctrl_mem.addr;
> > +hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
> > +
> > +return addr >= low && addr < hi;
> > +}
> > +
> > static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> > {
> > -if (n->cmbsz && addr >= n->ctrl_mem.addr &&
> > -addr < (n->ctrl_mem.addr +
> > int128_get64(n->ctrl_mem.size))) {
> > +if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
> > memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
> > -} else {
> > -pci_dma_read(>parent_obj, addr, buf, size);
> > +return;
> > }
> > +
> > +pci_dma_read(>parent_obj, addr, buf, size);
> > }
> >
> > static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
>
> Note that this patch still contains a bug that it removes the check against
> the accessed
> size, which you fix in later patch.
> I prefer to not add a bug in first place
> However if you have a reason for this, I won't mind.
>
So yeah. The resons is that there is actually no bug at this point
because the controller only supports PRPs. I actually thought there was
a bug as well and reported it to qemu-security some months ago as a
potential out of bounds access. I was then schooled by Keith on how PRPs
work ;) Below is a paraphrased version of Keiths analysis.
The PRPs does not cross page boundaries:
trans_len = n->page_size - (prp1 % n->page_size);
The PRPs are always verified to be page aligned:
if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
and the transfer length wont go above page size. So, since the beginning
of the address is within the CMB and considering that the CMB is of an
MB aligned and sized granularity, then we can never cross outside it
with PRPs.
I could add the check at this point (because it *is* needed for when
SGLs are introduced), but I think it would just be noise and I would
need to explain why the check is there, but not really needed at this
point. Instead I'm adding a new patch before the SGL patch that explains
this.
Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read
On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Pull the controller memory buffer check to its own function. The check
> will be used on its own in later patches.
>
> Signed-off-by: Klaus Jensen
> Acked-by: Keith Busch
> ---
> hw/block/nvme.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index b38d7e548a60..08a83d449de3 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -52,14 +52,22 @@
>
> static void nvme_process_sq(void *opaque);
>
> +static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> +{
> +hwaddr low = n->ctrl_mem.addr;
> +hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
> +
> +return addr >= low && addr < hi;
> +}
> +
> static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> {
> -if (n->cmbsz && addr >= n->ctrl_mem.addr &&
> -addr < (n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size))) {
> +if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
> memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
> -} else {
> -pci_dma_read(>parent_obj, addr, buf, size);
> +return;
> }
> +
> +pci_dma_read(>parent_obj, addr, buf, size);
> }
>
> static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
Note that this patch still contains a bug that it removes the check against the
accessed
size, which you fix in later patch.
I prefer to not add a bug in first place
However if you have a reason for this, I won't mind.
Best regards,
Maxim Levitsky
[PATCH v6 07/42] nvme: refactor nvme_addr_read
From: Klaus Jensen
Pull the controller memory buffer check to its own function. The check
will be used on its own in later patches.
Signed-off-by: Klaus Jensen
Acked-by: Keith Busch
---
hw/block/nvme.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b38d7e548a60..08a83d449de3 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -52,14 +52,22 @@
static void nvme_process_sq(void *opaque);
+static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
+{
+hwaddr low = n->ctrl_mem.addr;
+hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
+
+return addr >= low && addr < hi;
+}
+
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
{
-if (n->cmbsz && addr >= n->ctrl_mem.addr &&
-addr < (n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size))) {
+if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
-} else {
-pci_dma_read(>parent_obj, addr, buf, size);
+return;
}
+
+pci_dma_read(>parent_obj, addr, buf, size);
}
static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
--
2.25.1
