Dan Carpenter
2023-Jun-21 09:47 UTC
[Bridge] [PATCH 06/11] sysctl: Add size to register_net_sysctl function
The patchset doesn't include the actual interesting changes, just a bunch of mechanical prep work. On Wed, Jun 21, 2023 at 11:09:55AM +0200, Joel Granados wrote:> diff --git a/net/ieee802154/6lowpan/reassembly.c b/net/ieee802154/6lowpan/reassembly.c > index a91283d1e5bf..7b717434368c 100644 > --- a/net/ieee802154/6lowpan/reassembly.c > +++ b/net/ieee802154/6lowpan/reassembly.c > @@ -379,7 +379,8 @@ static int __net_init lowpan_frags_ns_sysctl_register(struct net *net) > table[1].extra2 = &ieee802154_lowpan->fqdir->high_thresh; > table[2].data = &ieee802154_lowpan->fqdir->timeout; > > - hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table); > + hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table, > + ARRAY_SIZE(lowpan_frags_ns_ctl_table));For example, in lowpan_frags_ns_sysctl_register() the sentinel is sometimes element zero if the user doesn't have enough permissions. I would want to ensure that was handled correctly, but that's going to be done later in a completely different patchset. I'm definitely not going to remember to check.> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c > index dc5165d3eec4..6f96aae76537 100644 > --- a/net/mpls/af_mpls.c > +++ b/net/mpls/af_mpls.c > @@ -1395,6 +1395,40 @@ static const struct ctl_table mpls_dev_table[] = { > { } > }; > > +static int mpls_platform_labels(struct ctl_table *table, int write, > + void *buffer, size_t *lenp, loff_t *ppos); > +#define MPLS_NS_SYSCTL_OFFSET(field) \ > + (&((struct net *)0)->field) > + > +static const struct ctl_table mpls_table[] = { > + { > + .procname = "platform_labels", > + .data = NULL, > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = mpls_platform_labels, > + }, > + { > + .procname = "ip_ttl_propagate", > + .data = MPLS_NS_SYSCTL_OFFSET(mpls.ip_ttl_propagate), > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = proc_dointvec_minmax, > + .extra1 = SYSCTL_ZERO, > + .extra2 = SYSCTL_ONE, > + }, > + { > + .procname = "default_ttl", > + .data = MPLS_NS_SYSCTL_OFFSET(mpls.default_ttl), > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = proc_dointvec_minmax, > + .extra1 = SYSCTL_ONE, > + .extra2 = &ttl_max, > + }, > + { } > +}; > + > static int mpls_dev_sysctl_register(struct net_device *dev, > struct mpls_dev *mdev) > { > @@ -1410,7 +1444,7 @@ static int mpls_dev_sysctl_register(struct net_device *dev, > /* Table data contains only offsets relative to the base of > * the mdev at this point, so make them absolute. > */ > - for (i = 0; i < ARRAY_SIZE(mpls_dev_table); i++) { > + for (i = 0; i < ARRAY_SIZE(mpls_dev_table) - 1; i++) {Adding the " - 1" is just a gratuitous change. It's not required. It makes that patch more confusing to review. And you're just going to have to change it back to how it was if you remove the sentinel.> table[i].data = (char *)mdev + (uintptr_t)table[i].data; > table[i].extra1 = mdev; > table[i].extra2 = net; > @@ -1418,7 +1452,8 @@ static int mpls_dev_sysctl_register(struct net_device *dev, > > snprintf(path, sizeof(path), "net/mpls/conf/%s", dev->name); > > - mdev->sysctl = register_net_sysctl(net, path, table); > + mdev->sysctl = register_net_sysctl(net, path, table, > + ARRAY_SIZE(mpls_dev_table)); > if (!mdev->sysctl) > goto free; > > @@ -1432,6 +1467,7 @@ static int mpls_dev_sysctl_register(struct net_device *dev, > return -ENOBUFS; > } > > +Double blank line.> static void mpls_dev_sysctl_unregister(struct net_device *dev, > struct mpls_dev *mdev) > {regards, dan carpenter
Dan Carpenter
2023-Jun-21 10:23 UTC
[Bridge] [PATCH 06/11] sysctl: Add size to register_net_sysctl function
On Wed, Jun 21, 2023 at 12:47:30PM +0300, Dan Carpenter wrote:> The patchset doesn't include the actual interesting changes, just a > bunch of mechanical prep work. > > On Wed, Jun 21, 2023 at 11:09:55AM +0200, Joel Granados wrote: > > diff --git a/net/ieee802154/6lowpan/reassembly.c b/net/ieee802154/6lowpan/reassembly.c > > index a91283d1e5bf..7b717434368c 100644 > > --- a/net/ieee802154/6lowpan/reassembly.c > > +++ b/net/ieee802154/6lowpan/reassembly.c > > @@ -379,7 +379,8 @@ static int __net_init lowpan_frags_ns_sysctl_register(struct net *net) > > table[1].extra2 = &ieee802154_lowpan->fqdir->high_thresh; > > table[2].data = &ieee802154_lowpan->fqdir->timeout; > > > > - hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table); > > + hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table, > > + ARRAY_SIZE(lowpan_frags_ns_ctl_table)); > > For example, in lowpan_frags_ns_sysctl_register() the sentinel is > sometimes element zero if the user doesn't have enough permissions. I > would want to ensure that was handled correctly, but that's going to be > done later in a completely different patchset. I'm definitely not going > to remember to check.On reflecting the patch is obviously wrong. It should be pass zero as table_size in that case. See diff at the end. There is a similar bug in neigh_sysctl_register() where we use memset to zero out the whole table. And another in __ip_vs_lblc_init(). I used the smatch cross function database `smdb.py where ctl_table procname | grep '(null)' | grep min-max` to make a list of functions which set procname to zero. Probably we should add a WARN_ON() if procname is zero in the new code which doesn't use sentinels. regards, dan carpenter drivers/char/random.c | proc_do_uuid | (struct ctl_table)->procname | 0 fs/proc/proc_sysctl.c | new_dir | (struct ctl_table)->procname | 48,3906148897379000352 fs/proc/proc_sysctl.c | new_links | (struct ctl_table)->procname | 4096-ptr_max arch/arm64/kernel/fpsimd.c | vec_proc_do_default_vl | (struct ctl_table)->procname | 0 arch/arm64/kernel/armv8_deprecated.c | register_insn_emulation | (struct ctl_table)->procname | 0-u64max kernel/sysctl-test.c | sysctl_test_api_dointvec_null_tbl_data | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_api_dointvec_table_maxlen_unset | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_api_dointvec_table_len_is_zero | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_api_dointvec_table_read_but_position_set | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_dointvec_read_happy_single_positive | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_dointvec_read_happy_single_negative | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_dointvec_write_happy_single_positive | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_dointvec_write_happy_single_negative | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_api_dointvec_write_single_less_int_min | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl-test.c | sysctl_test_api_dointvec_write_single_greater_int_max | (struct ctl_table)->procname | 7612622206476333056 kernel/sysctl.c | proc_do_static_key | (struct ctl_table)->procname | 0 kernel/kexec_core.c | kexec_limit_handler | (struct ctl_table)->procname | 0 kernel/bpf/syscall.c | bpf_stats_handler | (struct ctl_table)->procname | 0 net/core/sysctl_net_core.c | rps_sock_flow_sysctl | (struct ctl_table)->procname | 0 net/core/sysctl_net_core.c | set_default_qdisc | (struct ctl_table)->procname | 0 net/core/neighbour.c | neigh_sysctl_register | (struct ctl_table)->procname | 0 net/netfilter/ipvs/ip_vs_lblc.c | __ip_vs_lblc_init | (struct ctl_table)->procname | 0-u64max net/netfilter/ipvs/ip_vs_lblcr.c | __ip_vs_lblcr_init | (struct ctl_table)->procname | 0-u64max net/netfilter/ipvs/ip_vs_ctl.c | proc_do_defense_mode | (struct ctl_table)->procname | 0 net/netfilter/ipvs/ip_vs_ctl.c | proc_do_sync_threshold | (struct ctl_table)->procname | 0 net/netfilter/ipvs/ip_vs_ctl.c | proc_do_sync_ports | (struct ctl_table)->procname | 0 net/netfilter/ipvs/ip_vs_ctl.c | ipvs_proc_est_nice | (struct ctl_table)->procname | 0 net/netfilter/ipvs/ip_vs_ctl.c | ipvs_proc_run_estimation | (struct ctl_table)->procname | 0 net/netfilter/ipvs/ip_vs_ctl.c | ip_vs_control_net_init_sysctl | (struct ctl_table)->procname | 0-u64max net/netfilter/nf_log.c | netfilter_log_sysctl_init | (struct ctl_table)->procname | 0-u64max net/sctp/sysctl.c | proc_sctp_do_hmac_alg | (struct ctl_table)->procname | 0 net/sctp/sysctl.c | proc_sctp_do_rto_min | (struct ctl_table)->procname | 0 net/sctp/sysctl.c | proc_sctp_do_rto_max | (struct ctl_table)->procname | 0 net/sctp/sysctl.c | proc_sctp_do_auth | (struct ctl_table)->procname | 0 net/sctp/sysctl.c | proc_sctp_do_udp_port | (struct ctl_table)->procname | 0 net/sctp/sysctl.c | proc_sctp_do_probe_interval | (struct ctl_table)->procname | 0 net/ipv6/route.c | ipv6_route_sysctl_init | (struct ctl_table)->procname | 0-u64max net/ipv6/addrconf.c | addrconf_sysctl_addr_gen_mode | (struct ctl_table)->procname | 0 net/ieee802154/6lowpan/reassembly.c | lowpan_frags_ns_sysctl_register | (struct ctl_table)->procname | 0-u64max net/xfrm/xfrm_sysctl.c | xfrm_sysctl_init | (struct ctl_table)->procname | 0-u64max net/phonet/sysctl.c | proc_local_port_range | (struct ctl_table)->procname | 0 net/ipv4/route.c | sysctl_route_net_init | (struct ctl_table)->procname | 0-u64max net/ipv4/sysctl_net_ipv4.c | ipv4_local_port_range | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | ipv4_privileged_ports | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | ipv4_ping_group_range | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_tcp_congestion_control | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_tcp_available_congestion_control | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_allowed_congestion_control | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_tcp_fastopen_key | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_tcp_available_ulp | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_tcp_ehash_entries | (struct ctl_table)->procname | 0 net/ipv4/sysctl_net_ipv4.c | proc_udp_hash_entries | (struct ctl_table)->procname | 0 diff --git a/net/ieee802154/6lowpan/reassembly.c b/net/ieee802154/6lowpan/reassembly.c index a91283d1e5bf..749238d38014 100644 --- a/net/ieee802154/6lowpan/reassembly.c +++ b/net/ieee802154/6lowpan/reassembly.c @@ -360,6 +360,7 @@ static int __net_init lowpan_frags_ns_sysctl_register(struct net *net) struct ctl_table_header *hdr; struct netns_ieee802154_lowpan *ieee802154_lowpan net_ieee802154_lowpan(net); + size_t table_size = ARRAY_SIZE(lowpan_frags_ns_ctl_table); table = lowpan_frags_ns_ctl_table; if (!net_eq(net, &init_net)) { @@ -369,8 +370,10 @@ static int __net_init lowpan_frags_ns_sysctl_register(struct net *net) goto err_alloc; /* Don't export sysctls to unprivileged users */ - if (net->user_ns != &init_user_ns) + if (net->user_ns != &init_user_ns) { table[0].procname = NULL; + table_size = 0; + } } table[0].data = &ieee802154_lowpan->fqdir->high_thresh; @@ -379,7 +382,7 @@ static int __net_init lowpan_frags_ns_sysctl_register(struct net *net) table[1].extra2 = &ieee802154_lowpan->fqdir->high_thresh; table[2].data = &ieee802154_lowpan->fqdir->timeout; - hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table); + hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table, table_size); if (hdr == NULL) goto err_reg;
Dan Carpenter
2023-Jun-21 10:49 UTC
[Bridge] [PATCH 06/11] sysctl: Add size to register_net_sysctl function
On Wed, Jun 21, 2023 at 12:47:30PM +0300, Dan Carpenter wrote:> The patchset doesn't include the actual interesting changes, just a > bunch of mechanical prep work. >I was wrong here, the patchset just hadn't all hit the mailing lists. I can't apply this patchset to anything. I tried linux-next, net, and net-next. So it's hard to review. It looks like ensure_safe_net_sysctl() never got update to use table_size... You could easily write a static checker test to print a warning any time that ->procname is checked for NULL. I have attached a Smatch check. You would need to added to check_list.h and recompile. net/sysctl_net.c:130 ensure_safe_net_sysctl() warn: checking ->procname 'ent->procname' regards, dan carpenter -------------- next part -------------- A non-text attachment was scrubbed... Name: check_checking_procname.c Type: text/x-csrc Size: 481 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/bridge/attachments/20230621/fe069539/attachment-0001.bin>
Joel Granados
2023-Jun-21 11:36 UTC
[Bridge] [PATCH 06/11] sysctl: Add size to register_net_sysctl function
On Wed, Jun 21, 2023 at 12:47:30PM +0300, Dan Carpenter wrote:> The patchset doesn't include the actual interesting changes, just a > bunch of mechanical prep work.Yep, The thread got mangled on the way out. But hopefully the rest of the patch made its way to the lists and maintainers.> > On Wed, Jun 21, 2023 at 11:09:55AM +0200, Joel Granados wrote: > > diff --git a/net/ieee802154/6lowpan/reassembly.c b/net/ieee802154/6lowpan/reassembly.c > > index a91283d1e5bf..7b717434368c 100644 > > --- a/net/ieee802154/6lowpan/reassembly.c > > +++ b/net/ieee802154/6lowpan/reassembly.c > > @@ -379,7 +379,8 @@ static int __net_init lowpan_frags_ns_sysctl_register(struct net *net) > > table[1].extra2 = &ieee802154_lowpan->fqdir->high_thresh; > > table[2].data = &ieee802154_lowpan->fqdir->timeout; > > > > - hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table); > > + hdr = register_net_sysctl(net, "net/ieee802154/6lowpan", table, > > + ARRAY_SIZE(lowpan_frags_ns_ctl_table)); > > For example, in lowpan_frags_ns_sysctl_register() the sentinel is > sometimes element zero if the user doesn't have enough permissions. I > would want to ensure that was handled correctly, but that's going to be > done later in a completely different patchset. I'm definitely not going > to remember to check.Very good catch! I have fixed this as well as ensure_safe_net_sysctl that was missing a table_size arg.> > > diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c > > index dc5165d3eec4..6f96aae76537 100644 > > --- a/net/mpls/af_mpls.c > > +++ b/net/mpls/af_mpls.c > > @@ -1395,6 +1395,40 @@ static const struct ctl_table mpls_dev_table[] = { > > { } > > }; > > > > +static int mpls_platform_labels(struct ctl_table *table, int write, > > + void *buffer, size_t *lenp, loff_t *ppos); > > +#define MPLS_NS_SYSCTL_OFFSET(field) \ > > + (&((struct net *)0)->field) > > + > > +static const struct ctl_table mpls_table[] = { > > + { > > + .procname = "platform_labels", > > + .data = NULL, > > + .maxlen = sizeof(int), > > + .mode = 0644, > > + .proc_handler = mpls_platform_labels, > > + }, > > + { > > + .procname = "ip_ttl_propagate", > > + .data = MPLS_NS_SYSCTL_OFFSET(mpls.ip_ttl_propagate), > > + .maxlen = sizeof(int), > > + .mode = 0644, > > + .proc_handler = proc_dointvec_minmax, > > + .extra1 = SYSCTL_ZERO, > > + .extra2 = SYSCTL_ONE, > > + }, > > + { > > + .procname = "default_ttl", > > + .data = MPLS_NS_SYSCTL_OFFSET(mpls.default_ttl), > > + .maxlen = sizeof(int), > > + .mode = 0644, > > + .proc_handler = proc_dointvec_minmax, > > + .extra1 = SYSCTL_ONE, > > + .extra2 = &ttl_max, > > + }, > > + { } > > +}; > > + > > static int mpls_dev_sysctl_register(struct net_device *dev, > > struct mpls_dev *mdev) > > { > > @@ -1410,7 +1444,7 @@ static int mpls_dev_sysctl_register(struct net_device *dev, > > /* Table data contains only offsets relative to the base of > > * the mdev at this point, so make them absolute. > > */ > > - for (i = 0; i < ARRAY_SIZE(mpls_dev_table); i++) { > > + for (i = 0; i < ARRAY_SIZE(mpls_dev_table) - 1; i++) { > > Adding the " - 1" is just a gratuitous change. It's not required. > It makes that patch more confusing to review. And you're just going > to have to change it back to how it was if you remove the sentinel.Removed this for convenience. Thx.> > > table[i].data = (char *)mdev + (uintptr_t)table[i].data; > > table[i].extra1 = mdev; > > table[i].extra2 = net; > > @@ -1418,7 +1452,8 @@ static int mpls_dev_sysctl_register(struct net_device *dev, > > > > snprintf(path, sizeof(path), "net/mpls/conf/%s", dev->name); > > > > - mdev->sysctl = register_net_sysctl(net, path, table); > > + mdev->sysctl = register_net_sysctl(net, path, table, > > + ARRAY_SIZE(mpls_dev_table)); > > if (!mdev->sysctl) > > goto free; > > > > @@ -1432,6 +1467,7 @@ static int mpls_dev_sysctl_register(struct net_device *dev, > > return -ENOBUFS; > > } > > > > +Oops. thx. fixed> > Double blank line. > > > static void mpls_dev_sysctl_unregister(struct net_device *dev, > > struct mpls_dev *mdev) > > { > > regards, > dan carpenter-- Joel Granados -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/bridge/attachments/20230621/d286eb64/attachment-0001.sig>