Hi Florian, thanks for the review.
On 07/12/2018 01:03 PM, Florian Westphal wrote:
Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx> wrote:
+extern struct list_head nft_osf_fingers[2];
How is this going to be used?
I find it weird to see this in netfilter core.
This list is used to load the OS signatures from pf.os.
+ f = nla_data(osf_attrs[OSF_ATTR_FINGER]);
+
+ kf = kmalloc(sizeof(struct nf_osf_finger), GFP_KERNEL);
+ if (!kf)
+ return -ENOMEM;
+
+ for (i = 0; i < ARRAY_SIZE(nft_osf_fingers); ++i)
+ INIT_LIST_HEAD(&nft_osf_fingers[i]);
+
This is missing input validation.
I see no nla_policy for OSF_ATTR_FINGER.
Userspace could have placed anything from 0 to 0xffff bytes.
+ memcpy(&kf->finger, f, sizeof(struct nf_osf_user_finger));
Probably should use
nla_memdup() + an nla_plolicy struct entry.
Or nla_memdup() plus manual checking of nla_len() vs. expected/sane
values?
Agree, will fix that.
+ list_for_each_entry(sf, &nft_osf_fingers[!!f->df], finger_entry) {
+ if (memcmp(&sf->finger, f, sizeof(struct nf_osf_user_finger)))
+ continue;
+
+ kfree(kf);
Hmm. So there can't be any duplicate entries in first place.
So I really wonder how this is going to be used or why
all of this code can't live in nft_osf.c .
I mean, we are adding this to core nftables api, and i think this is
something that should only be done if it can't be specific to particular
expression for some reason.
Can we load/unload pf.os signatures from nft_osf.c at the expression
init? If we can do it, then maybe all this code can live in nft_osf.c.
+ list_for_each_entry(sf, &nft_osf_fingers[!!f->df], finger_entry) {
+ if (memcmp(&sf->finger, f, sizeof(struct nf_osf_user_finger)))
+ continue;
list_for_each_entry_safe?
I think it is not necessary and isn't used in xt_osf module, but we can
use it if recommended.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
