| Message ID | 20180312231442.GA22071@embeddedgus (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2018-03-12 at 18:14 -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 [] > diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c [] > @@ -51,19 +51,27 @@ ctnl_timeout_parse_policy(void *timeouts, > const struct nf_conntrack_l4proto *l4proto, > struct net *net, const struct nlattr *attr) > { > + struct nlattr **tb; > int ret = 0; > > - if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) { > - struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1]; > + if (!l4proto->ctnl_timeout.nlattr_to_obj) > + return 0; Why not if unlikely(!...) > > - ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max, > - attr, l4proto->ctnl_timeout.nla_policy, > - NULL); > - if (ret < 0) > - return ret; > + tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb), > + GFP_KERNEL); kmalloc_array? > > - ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts); > - } > + if (!tb) > + return -ENOMEM; > + > + ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max, attr, > + l4proto->ctnl_timeout.nla_policy, NULL); > + if (ret < 0) > + goto err; > + > + ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts); > + > +err: > + kfree(tb); > return ret; > } >
On Mon, Mar 12, 2018 at 04:58:38PM -0700, Joe Perches wrote: > On Mon, 2018-03-12 at 18:14 -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 > [] > > diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c > [] > > @@ -51,19 +51,27 @@ ctnl_timeout_parse_policy(void *timeouts, > > const struct nf_conntrack_l4proto *l4proto, > > struct net *net, const struct nlattr *attr) > > { > > + struct nlattr **tb; > > int ret = 0; > > > > - if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) { > > - struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1]; > > + if (!l4proto->ctnl_timeout.nlattr_to_obj) > > + return 0; > > Why not > if unlikely(!...) This is control plane code - not packet path - I think we should just let the compiler decide on this one, not really need to provide an explicit hint here.
On Tue, 2018-03-13 at 15:59 +0100, Pablo Neira Ayuso wrote: > On Mon, Mar 12, 2018 at 04:58:38PM -0700, Joe Perches wrote: > > On Mon, 2018-03-12 at 18:14 -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 > > > > [] > > > diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c > > > > [] > > > @@ -51,19 +51,27 @@ ctnl_timeout_parse_policy(void *timeouts, > > > const struct nf_conntrack_l4proto *l4proto, > > > struct net *net, const struct nlattr *attr) > > > { > > > + struct nlattr **tb; > > > int ret = 0; > > > > > > - if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) { > > > - struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1]; > > > + if (!l4proto->ctnl_timeout.nlattr_to_obj) > > > + return 0; > > > > Why not > > if unlikely(!...) > > This is control plane code - not packet path - I think we should just > let the compiler decide on this one, not really need to provide an > explicit hint here. I don't have an issue with that, but it should probably be mentioned in the changelog as it's unrelated to VLA removal.
On Mon, Mar 12, 2018 at 06:14:42PM -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 Applied, thanks.
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c index 6819300..dcd7bd3 100644 --- a/net/netfilter/nfnetlink_cttimeout.c +++ b/net/netfilter/nfnetlink_cttimeout.c @@ -51,19 +51,27 @@ ctnl_timeout_parse_policy(void *timeouts, const struct nf_conntrack_l4proto *l4proto, struct net *net, const struct nlattr *attr) { + struct nlattr **tb; int ret = 0; - if (likely(l4proto->ctnl_timeout.nlattr_to_obj)) { - struct nlattr *tb[l4proto->ctnl_timeout.nlattr_max+1]; + if (!l4proto->ctnl_timeout.nlattr_to_obj) + return 0; - ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max, - attr, l4proto->ctnl_timeout.nla_policy, - NULL); - if (ret < 0) - return ret; + tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb), + GFP_KERNEL); - ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts); - } + if (!tb) + return -ENOMEM; + + ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max, attr, + l4proto->ctnl_timeout.nla_policy, NULL); + if (ret < 0) + goto err; + + ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts); + +err: + kfree(tb); return ret; }
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_cttimeout.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)