lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 30 Jul 2017 15:59:10 -0400 From: Jamal Hadi Salim <jhs@...atatu.com> To: Jiri Pirko <jiri@...nulli.us> Cc: davem@...emloft.net, netdev@...r.kernel.org, xiyou.wangcong@...il.com, eric.dumazet@...il.com, horms@...ge.net.au, dsahern@...il.com Subject: Re: [PATCH net-next v12 1/4] net netlink: Add new type NLA_BITFIELD32 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, jhs@...atatu.com wrote: >> From: Jamal Hadi Salim <jhs@...atatu.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 <jiri@...lanox.com> >> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.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 >>
Powered by blists - more mailing lists