Re: [RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Alexander Aring <alex.aring@gmail.com>]

On Tue, Aug 04, 2015 at 06:29:13PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 30/07/15 10:55, Alexander Aring wrote:
> >This patch introduce two static inline functions. The first to get the
> >frame control field from an sk_buff. The second is for checking on the
> >acknowledgment request bit on the frame control field. Later we can
> >introduce more functions to check on the frame control fields.
> >
> >These will deprecate the current behaviour which requires a
> >host-byteorder conversion and manually bit handling.
> >
> >Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> >---
> 
> Some language suggestions inside.

ok.
> >  include/linux/ieee802154.h | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> >diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
> >index 1dc1f4e..4f26c01 100644
> >--- a/include/linux/ieee802154.h
> >+++ b/include/linux/ieee802154.h
> >@@ -25,6 +25,8 @@
> >  #include <linux/types.h>
> >  #include <linux/random.h>
> >+#include <linux/skbuff.h>
> >+#include <linux/unaligned/memmove.h>
> >  #include <asm/byteorder.h>
> >  #define IEEE802154_MTU			127
> >@@ -205,6 +207,33 @@ enum {
> >  	IEEE802154_SCAN_IN_PROGRESS = 0xfc,
> >  };
> >+/* frame control handling */
> >+#define IEEE802154_FCTL_ACKREQ	0x0020
> >+
> >+/**
> >+ * ieee802154_is_ackreq - check if acknowledgment request bit is set
> >+ * @fc: frame control bytes in little-endian byteorder
> >+ */
> >+static inline bool ieee802154_is_ackreq(__le16 fc)
> >+{
> >+	return fc & cpu_to_le16(IEEE802154_FCTL_ACKREQ);
> >+}
> >+
> >+/**
> >+ * ieee802154_get_fc_from_skb - get the frame control field from an skb
> 
> ... from a skb

ok.
> >+ * @skb: skb where the frame control field will be get from
> Maybe:
> 
> skb which contains the frame control field
> 

ok.
> >+ */
> >+static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
> >+{
> >+	/* return some invalid fc on failure */
> Maybe:
> 
> return on invalid fc
> 

ok.
> >+	if (unlikely(skb->mac_len < 2)) {
> >+		WARN_ON(1);
> >+		return cpu_to_le16(0);
> >+	}
> >+
> >+	return (__force __le16)__get_unaligned_memmove16(skb_mac_header(skb));
> 
> Just to make sure we don't run into problems like we did with the 6lowpan
> stack. __get_unaligned_memmove16 is not pulling the fc bytes out of the skb,
> right? The skb stays as it is.
> 

right it doesn't manipulate the skb. For the "problems like we did with
the 6lowpan" you need to decide which problems, I see several:

 - running skb_pull (which removes) buffer and we don't have the room to
   pull out the bytes of skb, example: skb->len = 3, skb_pull size is 4
   which ends in a BUG(), we need to check it with skb_may_pull before.

 - running skb_push and we don't have the headroom for that. Like
   headroom space is 3 but we running push for size 4. the function
   skb_cow will reallocte headroom if needed.

These two problems are mostly out of 6lowpan code (I think).

The problem which I mentioned at iphc is more different. This is read
out the data but we doesn't check if we getting a buffer out of read
access. Example:

tmp a[3], b[4];

memcpy(b, a, ARRAY_SIZE(b));

And the source pointer will read something from the stack. But this is
handled here by checking "unlikely(skb->mac_len < 2)".

> >+}
> >+
> >  /**
> >   * ieee802154_is_valid_psdu_len - check if psdu len is valid
> >   * available lengths:
> 
> Given the above is true you have my:
> 

ok.

- Alex