From: Joel Granados <j.granados@samsung.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: mcgrof@kernel.org, linux-rdma@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-hams@vger.kernel.org, netfilter-devel@vger.kernel.org,
coreteam@netfilter.org, bridge@lists.linux-foundation.org,
dccp@vger.kernel.org, linux-wpan@vger.kernel.org,
mptcp@lists.linux.dev, lvs-devel@vger.kernel.org,
rds-devel@oss.oracle.com, linux-afs@lists.infradead.org,
linux-sctp@vger.kernel.org, linux-s390@vger.kernel.org,
tipc-discussion@lists.sourceforge.net, linux-x25@vger.kernel.org
Subject: Re: [PATCH 06/11] sysctl: Add size to register_net_sysctl function
Date: Fri, 23 Jun 2023 16:21:26 +0200 [thread overview]
Message-ID: <20230623142126.rgdabyokvuwshhmj@localhost> (raw)
In-Reply-To: <5aba7eee-7a6e-4f3b-9921-e4220d479346@kadam.mountain>
[-- Attachment #1: Type: text/plain, Size: 8548 bytes --]
Just answering to dan and the lists to avoid noise
On Wed, Jun 21, 2023 at 01:23:52PM +0300, Dan Carpenter wrote:
> 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
> net/netfilter/nf_log.c | netfilter_log_sysctl_init | (struct ctl_table)->procname | 0-u64max
^^^ : These are all false positives as non-sentinel procname gets
set to a string on the register sysctl path.
> 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/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/addrconf.c | addrconf_sysctl_addr_gen_mode | (struct ctl_table)->procname | 0
> net/phonet/sysctl.c | proc_local_port_range | (struct ctl_table)->procname | 0
> 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/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
^^^ : procname gets set to NULL but is not used to register. just as a
temp ctl_table. Can be ignored.
> net/core/neighbour.c | neigh_sysctl_register | (struct ctl_table)->procname | 0
^^^ : This is one is tricky. I had handled the case where the sentinel
is cleared by setting procname to NULL.
> 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
^^^ : These two where caught by your check. THX!!!!!!
> net/netfilter/ipvs/ip_vs_ctl.c | ip_vs_control_net_init_sysctl | (struct ctl_table)->procname | 0-u64max
^^^ : This one is another one that was caught.
For this file I noticed that we setup the entire ctl_table for both privileged
and unprivileged users. It seems that we could just skip all the setup
needed for the unprivileged user. Please tell me and I can maybe come
with a patch for that.
> net/ipv6/route.c | ipv6_route_sysctl_init | (struct ctl_table)->procname | 0-u64max
> 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/ipv4/route.c | sysctl_route_net_init | (struct ctl_table)->procname | 0-u64max
^^^ : caught by your check!
In total you caught 8 valid errors.
Thx again.
Best
--
Joel Granados
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
next prev parent reply other threads:[~2023-06-23 14:21 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230621091000.424843-1-j.granados@samsung.com>
[not found] ` <CGME20230621091022eucas1p1c097da50842b23e902e1a674e117e1aa@eucas1p1.samsung.com>
2023-06-21 9:09 ` [PATCH 06/11] sysctl: Add size to register_net_sysctl function Joel Granados
2023-06-21 9:47 ` Dan Carpenter
2023-06-21 10:23 ` Dan Carpenter
2023-06-21 12:03 ` Joel Granados
2023-06-23 14:21 ` Joel Granados [this message]
2023-06-21 10:49 ` Dan Carpenter
2023-06-21 11:49 ` Joel Granados
2023-06-21 11:36 ` Joel Granados
[not found] ` <CGME20230621094824eucas1p2b6adfbd3f15ff3665674917f419b25d3@eucas1p2.samsung.com>
2023-06-21 9:48 ` [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays Joel Granados
2023-06-21 11:16 ` Jani Nikula
2023-06-21 13:06 ` Joel Granados
2023-06-21 13:15 ` Jani Nikula
2023-06-21 13:43 ` Joel Granados
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230623142126.rgdabyokvuwshhmj@localhost \
--to=j.granados@samsung.com \
--cc=bridge@lists.linux-foundation.org \
--cc=coreteam@netfilter.org \
--cc=dan.carpenter@linaro.org \
--cc=dccp@vger.kernel.org \
--cc=linux-afs@lists.infradead.org \
--cc=linux-hams@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=linux-wpan@vger.kernel.org \
--cc=linux-x25@vger.kernel.org \
--cc=lvs-devel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=mptcp@lists.linux.dev \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=rds-devel@oss.oracle.com \
--cc=tipc-discussion@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).