Re: [PATCH v1 0/9] Fixes multiple sysctl bound checks
Nicolas Bouchinet <[email protected]>
| Newsgroups | gmane.linux.scsi,gmane.linux.kernel,gmane.linux.drivers.rdma,gmane.comp.file-systems.coda.general,gmane.linux.nfs,gmane.linux.network,gmane.comp.security.firewalls.netfilter.devel |
|---|---|
| Message-ID | <[email protected]> |
Hi, Thank's for your reply. On 1/27/25 19:05, Joe Damato wrote: > On Mon, Jan 27, 2025 at 03:19:57PM +0100, [email protected] wrote: >> From: Nicolas Bouchinet <[email protected]> >> >> Hi, >> >> This patchset adds some bound checks to sysctls to avoid negative >> value writes. >> >> The patched sysctls were storing the result of the proc_dointvec >> proc_handler into an unsigned int data. proc_dointvec being able to >> parse negative value, and it return value being a signed int, this could >> lead to undefined behaviors. >> This has led to kernel crash in the past as described in commit >> 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table") >> >> Most of them are now bounded between SYSCTL_ZERO and SYSCTL_INT_MAX. >> nf_conntrack_expect_max is bounded between SYSCTL_ONE and SYSCTL_INT_MAX >> as defined by its documentation. > I noticed that none of the patches have a Fixes tags. Do any of > these fix existing crashes or is this just cleanup? I've just saw that xfrm{4,6}_gc_thresh sysctls where obsolete since 4.14 in the documentation... Also, ipv4_dst_ops.gc_thresh is set to `~0` since commit 4ff3885262d0 ("ipv4: Delete routing cache."). Wich will be printed as -1 when this syctl is read. ``` $ cat /proc/sys/net/ipv4/route/gc_thresh -1 ``` IIUC, it seems to be used in order to disable the garbage collection, hence, this patch would make it impossible to a user to disable it this way. It should thus be bounded it between SYSCTL_NEG_ONE and SYSCTL_INT_MAX. Your right, it's only cleanup, I'll push patch 3 separately only on netdev, with extended impact analyses, sorry for that. > > I am asking because if this is cleanup then it would be "net-next" > material instead of "net" and would need to be resubmit when then > merge window has passed [1]. > > FWIW, I submit a similar change some time ago and it was submit to > net-next as cleanup [2]. > > [1]: https://lore.kernel.org/netdev/[email protected]/ > [2]: https://lore.kernel.org/netdev/CANn89i+=HiffVo9iv2NKMC2LFT15xFLG16h7wN3MCrTiKT3zQQ@mail.gmail.com/T/ >