Re: [RFC v3 12/27] lib: rspdm: Support SPDM get_version

[Jonathan Cameron]

> 
> >  
> > > +            let addr = unaligned.wrapping_add(i as usize);
> > > +            let version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } >> 8) as u8;  
> > Given the endian conversion below, this is correct, but I think leaving it as the __le16 until
> > here is better.  We could also just pull out the correct byte and ignore the other one.  
> 
> Hmm... I'm not sure I follow.
> 
> The SPDM spec describes this as a u16, so I'm reading the entire value
> and just taking the upper bits for the Major/Minor version

Miss read by me I think... Or tied up with endian conversion I think you
say you are getting rid of below.


> 
> So I'm not sure what you think I should do differently?
> 
> > It's a spec quirk that they decided to have it defined as a __le16 rather than u8[2] as
> > the fields are confined one byte or the other.
> >
> > I wonder if we should also reject any alpha versions? I.e. check the bottom 4 bits
> > are 0.  
> 
> Hmmm... That seems a bit unnecessary. Maybe a warning though?

Warning works for me.

> 
> >  
> > > +
> > > +            if version >= self.version && version <= SPDM_MAX_VER {
> > > +                self.version = version;
> > > +                foundver = true;
> > > +            }
> > > +        }
> > > +
> > > +        if !foundver {
> > > +            pr_err!("No common supported version\n");
> > > +            to_result(-(bindings::EPROTO as i32))?;
> > > +        }
> > > +
> > > +        Ok(())
> > > +    }
> > >  }
> > > diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> > > index a0a3a2f46952..f69be6aa6280 100644
> > > --- a/lib/rspdm/validator.rs
> > > +++ b/lib/rspdm/validator.rs
> > > @@ -7,6 +7,7 @@
> > >  //! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> > >  //! <https://www.dmtf.org/dsp/DSP0274>
> > >
> > > +use crate::bindings::{__IncompleteArrayField, __le16};
> > >  use crate::consts::SpdmErrorCode;
> > >  use core::mem;
> > >  use kernel::prelude::*;
> > > @@ -15,6 +16,8 @@
> > >      validate::{Unvalidated, Validate},
> > >  };
> > >
> > > +use crate::consts::SPDM_GET_VERSION;
> > > +
> > >  #[repr(C, packed)]
> > >  pub(crate) struct SpdmHeader {
> > >      pub(crate) version: u8,
> > > @@ -64,3 +67,67 @@ pub(crate) struct SpdmErrorRsp {
> > >      pub(crate) error_code: SpdmErrorCode,
> > >      pub(crate) error_data: u8,
> > >  }
> > > +
> > > +#[repr(C, packed)]  
> >
> > Why packed?  
> 
> We cast it from a struct to a slice (array), so we need it to be
> packed to avoid gaps in the slice (array)
Ah ok.  I'm learning slowly...
> 
> >  
> > > +pub(crate) struct GetVersionReq {
> > > +    pub(crate) version: u8,
> > > +    pub(crate) code: u8,
> > > +    pub(crate) param1: u8,
> > > +    pub(crate) param2: u8,
> > > +}
> > > +
> > > +        // undefined behaviour, so we operate on the raw data directly
> > > +        let unaligned = core::ptr::addr_of_mut!(rsp.version_number_entries) as *mut u16;
> > > +        for version_offset in 0..version_number_entries {
> > > +            let addr = unaligned.wrapping_add(version_offset);
> > > +            let version = unsafe { core::ptr::read_unaligned::<u16>(addr) };
> > > +            unsafe { core::ptr::write_unaligned::<u16>(addr, version.to_le()) }  
> >
> > I'd like to see a comment on why this seems to be doing an endian swap if we
> > are on big endian. Looking back at the c code, it has an endian swap but
> > only as part of the search for a version to use (finding the largest both
> > device and and kernel support).
> >
> > Maybe I'm reading it wrong but isn't this putting cpu endian data into a structure
> > element that is of type __le16?  
> 
> On second thought I don't think this is required at all. It's
> converting the LE data from the SPDM response to LE. Which is just
> unnecessary no-op (LE) or a bug (BE) depending on the CPU endianness.
I think that makes sense but I'll take a close look at v4.

J
> 
> Alistair
> 
> >  
> > > +        }
> > > +
> > > +        Ok(rsp)
> > > +    }
> > > +}  
> >