Jiri,

This is getting exhausting, seriously.
I posted the code you are commenting one two days ago so i dont have to
repost.

On D. Ahern: I dont think we are disagreeing anymore on the need to
generalize the check. He is saying it should be a helper and I already
had the validation data; either works. I dont see the gapping need
to remove the validation data.

cheers,
jamal

On 17-07-30 02:42 PM, Jiri Pirko wrote:
Sun, Jul 30, 2017 at 07:24:49PM CEST, j...@mojatatu.com wrote:
From: Jamal Hadi Salim <j...@mojatatu.com>

Generic bitflags attribute content sent to the kernel by user.
With this netlink attr type the user can either set or unset a
flag in the kernel.

The value is a bitmap that defines the bit values being set
The selector is a bitmask that defines which value bit is to be
considered.

A check is made to ensure the rules that a kernel subsystem always
conforms to bitflags the kernel already knows about. i.e
if the user tries to set a bit flag that is not understood then
the _it will be rejected_.

In the most basic form, the user specifies the attribute policy as:
[ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data = &myvalidflags },

where myvalidflags is the bit mask of the flags the kernel understands.

If the user _does not_ provide myvalidflags then the attribute will
also be rejected.

Examples:
value = 0x0, and selector = 0x1
implies we are selecting bit 1 and we want to set its value to 0.

value = 0x2, and selector = 0x2
implies we are selecting bit 2 and we want to set its value to 1.

Suggested-by: Jiri Pirko <j...@mellanox.com>
Signed-off-by: Jamal Hadi Salim <j...@mojatatu.com>
---
include/net/netlink.h        | 16 ++++++++++++++++
include/uapi/linux/netlink.h | 17 +++++++++++++++++
lib/nlattr.c                 | 30 ++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index ef8e6c3..82dd298 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -178,6 +178,7 @@ enum {
        NLA_S16,
        NLA_S32,
        NLA_S64,
+       NLA_BITFIELD32,
        __NLA_TYPE_MAX,
};

@@ -206,6 +207,7 @@ enum {
  *    NLA_MSECS            Leaving the length field zero will verify the
  *                         given type fits, using it verifies minimum length
  *                         just like "All other"
+ *    NLA_BITFIELD32      A 32-bit bitmap/bitselector attribute
  *    All other            Minimum length of attribute payload
  *
  * Example:
@@ -213,11 +215,13 @@ enum {
  *     [ATTR_FOO] = { .type = NLA_U16 },
  *     [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
  *     [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ *     [ATTR_GOO] = { .type = NLA_BITFIELD32, .validation_data = &myvalidflags 
},

Checkpatch warns you about the line to long, please wrap it.

Btw, I did not see you reached a consensus with DavidA regarding this.
Did I miss it?


  * };
  */
struct nla_policy {
        u16             type;
        u16             len;
+       void            *validation_data;
};

/**
@@ -1203,6 +1207,18 @@ static inline struct in6_addr nla_get_in6_addr(const 
struct nlattr *nla)
}

/**
+ * nla_get_bitfield32 - return payload of 32 bitfield attribute
+ * @nla: nla_bitfield32 attribute
+ */
+static inline struct nla_bitfield32 nla_get_bitfield32(const struct nlattr 
*nla)
+{
+       struct nla_bitfield32 tmp;
+
+       nla_memcpy(&tmp, nla, sizeof(tmp));
+       return tmp;
+}
+
+/**
  * nla_memdup - duplicate attribute memory (kmemdup)
  * @src: netlink attribute to duplicate from
  * @gfp: GFP mask
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index f86127a..f4fc9c9 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -226,5 +226,22 @@ struct nlattr {
#define NLA_ALIGN(len)          (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
#define NLA_HDRLEN              ((int) NLA_ALIGN(sizeof(struct nlattr)))

+/* Generic 32 bitflags attribute content sent to the kernel.
+ *
+ * The value is a bitmap that defines the values being set
+ * The selector is a bitmask that defines which value is legit
+ *
+ * Examples:
+ *  value = 0x0, and selector = 0x1
+ *  implies we are selecting bit 1 and we want to set its value to 0.
+ *
+ *  value = 0x2, and selector = 0x2
+ *  implies we are selecting bit 2 and we want to set its value to 1.
+ *
+ */
+struct nla_bitfield32 {
+       __u32 value;
+       __u32 selector;
+};

#endif /* _UAPI__LINUX_NETLINK_H */
diff --git a/lib/nlattr.c b/lib/nlattr.c
index fb52435..ee79b7a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -27,6 +27,30 @@
        [NLA_S64]       = sizeof(s64),
};

+static int validate_nla_bitfield32(const struct nlattr *nla,
+                                  u32 *valid_flags_allowed)
+{
+       const struct nla_bitfield32 *bf = nla_data(nla);
+       u32 *valid_flags_mask = valid_flags_allowed;

I pointed this out already. This weird.
You do *u32 = *u32, just with different name. Just use valid_flags_allowed
directly.


+
+       if (!valid_flags_allowed)
+               return -EINVAL;
+
+       /*disallow invalid bit selector */

Fix all the comments in this function. Should be
        /* something */
with spaces in front and at the end.


+       if (bf->selector & ~*valid_flags_mask)
+               return -EINVAL;
+
+       /*disallow invalid bit values */
+       if (bf->value & ~*valid_flags_mask)
+               return -EINVAL;
+
+       /*disallow valid bit values that are not selected*/
+       if (bf->value & ~bf->selector)
+               return -EINVAL;
+
+       return 0;
+}
+
static int validate_nla(const struct nlattr *nla, int maxtype,
                        const struct nla_policy *policy)
{
@@ -46,6 +70,12 @@ static int validate_nla(const struct nlattr *nla, int 
maxtype,
                        return -ERANGE;
                break;

+       case NLA_BITFIELD32:
+               if (attrlen != sizeof(struct nla_bitfield32))
+                       return -ERANGE;
+
+               return validate_nla_bitfield32(nla, pt->validation_data);
+
        case NLA_NUL_STRING:
                if (pt->len)
                        minlen = min_t(int, attrlen, pt->len + 1);
--
1.9.1


Reply via email to