Re: [RFC PATCH v2 10/18] calipso: Set the calipso socket label to match the secattr.

[Huw Davies]

On Sun, Feb 07, 2016 at 02:56:08PM -0500, Paul Moore wrote:
> On Friday, January 08, 2016 09:52:46 AM Huw Davies wrote:
> > +/**
> > + * calipso_genopt - Generate a CALIPSO option
> > + * @buf: the option buffer
> > + * @start: offset from which to write
> > + * @buf_len: the size of opt_buf
> > + * @doi_def: the CALIPSO DOI to use
> > + * @secattr: the security attributes
> > + *
> > + * Description:
> > + * Generate a CALIPSO option using the DOI definition and security
> > attributes + * passed to the function. This also generates upto three bytes
> > of leading + * padding that ensures that the option is 4n + 2 aligned.  It
> > returns the + * number of bytes written (including any initial padding).
> > + */
> > +static int calipso_genopt(unsigned char *buf, u32 start, u32 buf_len,
> > + const struct calipso_doi *doi_def,
> > + const struct netlbl_lsm_secattr *secattr)
> > +{
> > +   int ret_val;
> > +   u32 len, pad;
> > +   u16 crc;
> > +   static const unsigned char padding[4] = {2, 1, 0, 3};
> > +   unsigned char *calipso;
> > +
> > +   /* CALIPSO has 4n + 2 alignment */
> > +   pad = padding[start % 4];
> 
> It's probably quicker to use a bitmask than a modulus operation.

Ok.
 
> > +   if (buf_len <= start + pad + CALIPSO_HDR_LEN)
> > +   return -ENOSPC;
> > +
> > +   if ((secattr->flags & NETLBL_SECATTR_MLS_LVL) == 0)
> > +   return -EPERM;
> > +
> > +   len = CALIPSO_HDR_LEN;
> > +
> > +   if (secattr->flags & NETLBL_SECATTR_MLS_CAT) {
> > +   ret_val = calipso_map_cat_hton(doi_def,
> > +  secattr,
> > +  buf + start + pad + len,
> > +  buf_len - start - pad - len);
> > +   if (ret_val < 0)
> > +   return ret_val;
> > +   len += ret_val;
> > +   }
> > +
> > +   calipso_pad_write(buf, start, pad);
> > +   calipso = buf + start + pad;
> 
> Help me understand why we would need to pad before the CALIPSO option?  
> Assuming any preceding options are properly aligned, or there are no 
> preceding 
> options at all, this should never be needed, yes?

Options may end at any alignment, so the preceding option might end at
4n+1, say, followed by three bytes of padding.  What we'd like to do
is replace those three bytes with one byte and start the CALIPSO
option at 4n+2.  So in this case, we need to write a single padding
byte before the CALIPSO option.
 
> > +   calipso[0] = IPV6_TLV_CALIPSO;
> > +   calipso[1] = len - 2;
> > +   *(__be32 *)(calipso + 2) = htonl(doi_def->doi);
> > +   calipso[6] = (len - CALIPSO_HDR_LEN) / 4;
> > +   calipso[7] = secattr->attr.mls.lvl,
> > +   crc = crc_ccitt(0x, calipso, len);
> > +   crc = ~crc;
> 
> Why not just "crc = ~crc_ccitt(...);"?

No reason, will fix.
 
> > +   calipso[8] = crc & 0xff;
> > +   calipso[9] = (crc >> 8) & 0xff;
> 
> Do we need to convert the checksum to network byte order?

Not according to https://tools.ietf.org/html/rfc1662#section-3.1
(which is referenced from rfc 5570 when describing the CRC-16):

 Frame Check Sequence (FCS) Field

  The Frame Check Sequence field defaults to 16 bits (two octets).
  The FCS is transmitted least significant octet first, which
  contains the coefficient of the highest term.

Also, I obtained a packet capture from a Trusted Solaris box, and
this appears to be consistant with that.
 
> > +/**
> > + * calipso_tlv_len - Returns the length of the TLV.
> > + * @tlv: the TLV
> > + *
> > + * Description:
> > + * Returns the length of the provided TLV option.
> > + */
> > +static int calipso_tlv_len(unsigned char *tlv)
> > +{
> > +   if (tlv[0] == IPV6_TLV_PAD1)
> > +   return 1;
> > +   return tlv[1] + 2;
> > +}
> 
> Perhaps rename this to calipso_opt_len() and change the argument from tlv to 
> opt?

That would be confusing w.r.t. ipv6_optlen().  By calling it _tlv_len I'm
trying to distinguish the length of a TLV value from the length of the
entire hop option.  Other suggestions are welcome.
 
> There may also be a desire to make this a generic ipv6 function; to be honest 
> I'm surprised there isn't one to do this already.

I'll leave it internal for now; if the network guys want it, they can shout.

Actually, I need to tweak this function.  We can't guarantee that we can
access tlv[1] without knowing the hop option length.  I'm thinking of
something like this, which also checks that the entire TLV value
sits within the hop opt.

static int calipso_tlv_len(struct ipv6_opt_hdr *opt, unsigned int offset)
{
unsigned char *tlv = (unsigned char *)opt;
unsigned int opt_len = ipv6_optlen(opt), tlv_len;

if (offset < sizeof(*opt) || offset >= opt_len)
return -EINVAL;
if (tlv[offset] == IPV6_TLV_PAD1)
return 1;
if (offset + 1 >= opt_len)
return -EINVAL;
tlv_len = tlv[offset + 1] + 2;

Re: [RFC PATCH v2 10/18] calipso: Set the calipso socket label to match the secattr.

[Paul Moore]

On Friday, January 08, 2016 09:52:46 AM Huw Davies wrote:
> CALIPSO is a hop-by-hop IPv6 option.  A lot of this patch is based on
> the equivalent CISPO code.  The main difference is due to manipulating
> the options in the hop-by-hop header.
> 
> Signed-off-by: Huw Davies 

...

> diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> index d7df7a4..ce803e2 100644
> --- a/net/ipv6/calipso.c
> +++ b/net/ipv6/calipso.c
> @@ -44,6 +44,17 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +/* Maximium size of the calipso option including
> + * the two-byte TLV header.
> + */
> +#define CALIPSO_OPT_LEN_MAX (2 + 252)
> +
> +/* Size of the minimum calipso option including
> + * the two-byte TLV header.
> + */
> +#define CALIPSO_HDR_LEN (2 + 8)
> 
>  /* List of available DOI definitions */
>  static DEFINE_SPINLOCK(calipso_doi_list_lock);
> @@ -297,6 +308,584 @@ doi_walk_return:
>   return ret_val;
>  }
> 
> +/**
> + * calipso_map_cat_hton - Perform a category mapping from host to network
> + * @doi_def: the DOI definition
> + * @secattr: the security attributes
> + * @net_cat: the zero'd out category bitmap in network/CIPSO format

I think that should be CALIPSO ;)

I didn't notice that mistake anywhere else but it is very possible I missed 
it; a quick 'grep -i "cipso"' on your patches might be a good idea.

> +/**
> + * calipso_genopt - Generate a CALIPSO option
> + * @buf: the option buffer
> + * @start: offset from which to write
> + * @buf_len: the size of opt_buf
> + * @doi_def: the CALIPSO DOI to use
> + * @secattr: the security attributes
> + *
> + * Description:
> + * Generate a CALIPSO option using the DOI definition and security
> attributes + * passed to the function. This also generates upto three bytes
> of leading + * padding that ensures that the option is 4n + 2 aligned.  It
> returns the + * number of bytes written (including any initial padding).
> + */
> +static int calipso_genopt(unsigned char *buf, u32 start, u32 buf_len,
> +   const struct calipso_doi *doi_def,
> +   const struct netlbl_lsm_secattr *secattr)
> +{
> + int ret_val;
> + u32 len, pad;
> + u16 crc;
> + static const unsigned char padding[4] = {2, 1, 0, 3};
> + unsigned char *calipso;
> +
> + /* CALIPSO has 4n + 2 alignment */
> + pad = padding[start % 4];

It's probably quicker to use a bitmask than a modulus operation.

> + if (buf_len <= start + pad + CALIPSO_HDR_LEN)
> + return -ENOSPC;
> +
> + if ((secattr->flags & NETLBL_SECATTR_MLS_LVL) == 0)
> + return -EPERM;
> +
> + len = CALIPSO_HDR_LEN;
> +
> + if (secattr->flags & NETLBL_SECATTR_MLS_CAT) {
> + ret_val = calipso_map_cat_hton(doi_def,
> +secattr,
> +buf + start + pad + len,
> +buf_len - start - pad - len);
> + if (ret_val < 0)
> + return ret_val;
> + len += ret_val;
> + }
> +
> + calipso_pad_write(buf, start, pad);
> + calipso = buf + start + pad;

Help me understand why we would need to pad before the CALIPSO option?  
Assuming any preceding options are properly aligned, or there are no preceding 
options at all, this should never be needed, yes?

> + calipso[0] = IPV6_TLV_CALIPSO;
> + calipso[1] = len - 2;
> + *(__be32 *)(calipso + 2) = htonl(doi_def->doi);
> + calipso[6] = (len - CALIPSO_HDR_LEN) / 4;
> + calipso[7] = secattr->attr.mls.lvl,
> + crc = crc_ccitt(0x, calipso, len);
> + crc = ~crc;

Why not just "crc = ~crc_ccitt(...);"?

> + calipso[8] = crc & 0xff;
> + calipso[9] = (crc >> 8) & 0xff;

Do we need to convert the checksum to network byte order?

> + return pad + len;
> +}
> +
> +/* Hop-by-hop hdr helper functions
> + */
> +
> +/**
> + * calipso_opt_update - Replaces socket's hop options with a new set
> + * @sk: the socket
> + * @hop: new hop options
> + *
> + * Description:
> + * Replaces @sk's hop options with @hop.  @hop may be NULL to leave
> + * the socket with no hop options.
> + *
> + */
> +static int calipso_opt_update(struct sock *sk, struct ipv6_opt_hdr *hop)
> +{
> + struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
> +
> + txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
> +  hop, hop ? ipv6_optlen(hop) : 0);
> + txopt_put(old);
> + if (IS_ERR(txopts))
> + return PTR_ERR(txopts);
> +
> + txopts = ipv6_update_options(sk, txopts);
> +
> + if (txopts) {
> + atomic_sub(txopts->tot_len, >sk_omem_alloc);
> + txopt_put(txopts);
> + }

Vertical whitespace between the txopts assignment and the if conditional.

> + return 0;
> +}
> +
> +/**
> + * calipso_tlv_len - Returns the length of the TLV.
> + * @tlv: the TLV
> + *
> + * Description:
> + *