| Message ID | 20180313002138.GA27280@embeddedgus (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 12, 2018 at 07:21:38PM -0500, Gustavo A. R. Silva wrote: > In preparation to enabling -Wvla, remove VLA and replace it > with dynamic memory allocation. > > From a security viewpoint, the use of Variable Length Arrays can be > a vector for stack overflow attacks. Also, in general, as the code > evolves it is easy to lose track of how big a VLA can get. Thus, we > can end up having segfaults that are hard to debug. > > Also, fixed as part of the directive to remove all VLAs from > the kernel: https://lkml.org/lkml/2018/3/7/621 also applied, thanks.
On 03/20/2018 07:36 AM, Pablo Neira Ayuso wrote: > On Mon, Mar 12, 2018 at 07:21:38PM -0500, Gustavo A. R. Silva wrote: >> In preparation to enabling -Wvla, remove VLA and replace it >> with dynamic memory allocation. >> >> From a security viewpoint, the use of Variable Length Arrays can be >> a vector for stack overflow attacks. Also, in general, as the code >> evolves it is easy to lose track of how big a VLA can get. Thus, we >> can end up having segfaults that are hard to debug. >> >> Also, fixed as part of the directive to remove all VLAs from >> the kernel: https://lkml.org/lkml/2018/3/7/621 > > also applied, thanks. > Awesome. Thanks, Pablo. -- Gustavo
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c index d33ce6d..4a4b293 100644 --- a/net/netfilter/nfnetlink_cthelper.c +++ b/net/netfilter/nfnetlink_cthelper.c @@ -314,23 +314,30 @@ nfnl_cthelper_update_policy_one(const struct nf_conntrack_expect_policy *policy, static int nfnl_cthelper_update_policy_all(struct nlattr *tb[], struct nf_conntrack_helper *helper) { - struct nf_conntrack_expect_policy new_policy[helper->expect_class_max + 1]; + struct nf_conntrack_expect_policy *new_policy; struct nf_conntrack_expect_policy *policy; - int i, err; + int i, ret = 0; + + new_policy = kmalloc_array(helper->expect_class_max + 1, + sizeof(*new_policy), GFP_KERNEL); + if (!new_policy) + return -ENOMEM; /* Check first that all policy attributes are well-formed, so we don't * leave things in inconsistent state on errors. */ for (i = 0; i < helper->expect_class_max + 1; i++) { - if (!tb[NFCTH_POLICY_SET + i]) - return -EINVAL; + if (!tb[NFCTH_POLICY_SET + i]) { + ret = -EINVAL; + goto err; + } - err = nfnl_cthelper_update_policy_one(&helper->expect_policy[i], + ret = nfnl_cthelper_update_policy_one(&helper->expect_policy[i], &new_policy[i], tb[NFCTH_POLICY_SET + i]); - if (err < 0) - return err; + if (ret < 0) + goto err; } /* Now we can safely update them. */ for (i = 0; i < helper->expect_class_max + 1; i++) { @@ -340,7 +347,9 @@ static int nfnl_cthelper_update_policy_all(struct nlattr *tb[], policy->timeout = new_policy->timeout; } - return 0; +err: + kfree(new_policy); + return ret; } static int nfnl_cthelper_update_policy(struct nf_conntrack_helper *helper,
In preparation to enabling -Wvla, remove VLA and replace it with dynamic memory allocation. From a security viewpoint, the use of Variable Length Arrays can be a vector for stack overflow attacks. Also, in general, as the code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having segfaults that are hard to debug. Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- net/netfilter/nfnetlink_cthelper.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)