Re: [PATCH v6 31/42] nvme: add check for prinfo

[Maxim Levitsky]

On Tue, 2020-03-31 at 07:45 +0200, Klaus Birkelund Jensen wrote:
> On Mar 25 12:57, Maxim Levitsky wrote:
> > On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > > From: Klaus Jensen <address@hidden>
> > > 
> > > Check the validity of the PRINFO field.
> > > 
> > > Signed-off-by: Klaus Jensen <address@hidden>
> > > ---
> > >  hw/block/nvme.c       | 50 ++++++++++++++++++++++++++++++++++++-------
> > >  hw/block/trace-events |  1 +
> > >  include/block/nvme.h  |  1 +
> > >  3 files changed, 44 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 7d5340c272c6..0d2b5b45b0c5 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -505,6 +505,17 @@ static inline uint16_t nvme_check_mdts(NvmeCtrl *n, 
> > > size_t len,
> > >      return NVME_SUCCESS;
> > >  }
> > >  
> > > +static inline uint16_t nvme_check_prinfo(NvmeCtrl *n, NvmeNamespace *ns,
> > > +                                         uint16_t ctrl, NvmeRequest *req)
> > > +{
> > > +    if ((ctrl & NVME_RW_PRINFO_PRACT) && !(ns->id_ns.dps & 
> > > DPS_TYPE_MASK)) {
> > > +        trace_nvme_dev_err_prinfo(nvme_cid(req), ctrl);
> > > +        return NVME_INVALID_FIELD | NVME_DNR;
> > > +    }
> > 
> > I refreshed my (still very limited) knowelege on the metadata
> > and the protection info, and this is what I found:
> > 
> > I think that this is very far from complete, because we also have:
> > 
> > 1. PRCHECK. According to the spec it is independent of PRACT
> >    And when some of it is set, 
> >    together with enabled protection (the DPS field in namespace),
> >    Then the 8 bytes of the protection info is checked (optionally using the
> >    the EILBRT and ELBAT/ELBATM fields in the command and CRC of the data 
> > for the guard field)
> > 
> >    So this field should also be checked to be zero when protection is 
> > disabled
> >    (I don't see an explicit requirement for that in the spec, but neither I 
> > see
> >    such requirement for PRACT)
> > 
> > 2. The protection values to be written / checked ((E)ILBRT/(E)LBATM/(E)LBAT)
> >    Same here, but also these should not be set when PRCHECK is not set for 
> > reads,
> >    plus some are protection type specific.
> > 
> > 
> > The spec does mention the 'Invalid Protection Information' error code which
> > refers to invalid values in the PRINFO field.
> > So this error code I think should be returned instead of the 'Invalid field'
> > 
> > Another thing to optionaly check is that the metadata pointer for separate 
> > metadata.
> >  Is zero as long as we don't support metadata
> > (again I don't see an explicit requirement for this in the spec, but it 
> > mentions:
> > 
> > "This field is valid only if the command has metadata that is not 
> > interleaved with
> > the logical block data, as specified in the Format NVM command"
> > 
> > )
> > 
> 
> I'm kinda inclined to just drop this patch. The spec actually says that
> the PRACT and PRCHK fields are used only if the namespace is formatted
> to use end-to-end protection information. Since we do not support that,
> I don't think we even need to check it.
> 
> Any opinion on this?
Yep. I also think so.

I did add as much as possible checks on all the reserved fields in nvme-mdev,
checks for all all the unused fields like that, to make it as defensive as 
possible and to reduce
the attack surface to the minimum.
This can be done later when all the dust settles, its not a high priority for 
sure.


Best regard,
        Maxim Levitsky


>