| Message ID | 20250127142014.37834-1-nicolas.bouchinet@clip-os.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fixes multiple sysctl bound checks | expand |
On Mon, Jan 27, 2025 at 03:19:57PM +0100, nicolas.bouchinet@clip-os.org wrote: > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > > 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 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/20250117182059.7ce1196f@kernel.org/ [2]: https://lore.kernel.org/netdev/CANn89i+=HiffVo9iv2NKMC2LFT15xFLG16h7wN3MCrTiKT3zQQ@mail.gmail.com/T/
On Mon, 27 Jan 2025 15:19:57 +0100 nicolas.bouchinet@clip-os.org wrote: > 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. > > This patchset has been written over sysctl-testing branch [1]. > See [2] for similar sysctl fixes currently in review. Please don't group patches for different subsystems in a series if there are no dependencies between them. Only patch 3 seems relevant for netdev@ / core networking. Please repost patch 3 separately with extended impact analysis and a Fixes tag (as requested by Joe).
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, nicolas.bouchinet@clip-os.org wrote: >> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> >> >> 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/20250117182059.7ce1196f@kernel.org/ > [2]: https://lore.kernel.org/netdev/CANn89i+=HiffVo9iv2NKMC2LFT15xFLG16h7wN3MCrTiKT3zQQ@mail.gmail.com/T/ >
From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> 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. This patchset has been written over sysctl-testing branch [1]. See [2] for similar sysctl fixes currently in review. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/sysctl/sysctl.git/log/?h=sysctl-testing [2]: https://lore.kernel.org/all/20250115132211.25400-1-nicolas.bouchinet@clip-os.org/ Best regards, Nicolas --- Nicolas Bouchinet (9): sysctl: Fixes nf_conntrack_max bounds sysctl: Fixes nf_conntrack_expect_max bounds sysctl: Fixes gc_thresh bounds sysctl: Fixes idmap_cache_timeout bounds sysctl: Fixes nsm_local_state bounds sysctl/coda: Fixes timeout bounds sysctl: Fixes scsi_logging_level bounds sysctl/infiniband: Fixes infiniband sysctl bounds sysctl: Fixes max-user-freq bounds drivers/char/hpet.c | 4 +++- drivers/infiniband/core/iwcm.c | 4 +++- drivers/infiniband/core/ucma.c | 4 +++- drivers/scsi/scsi_sysctl.c | 4 +++- fs/coda/sysctl.c | 4 +++- fs/lockd/svc.c | 4 +++- fs/nfs/nfs4sysctl.c | 4 +++- net/ipv4/route.c | 4 +++- net/ipv6/route.c | 4 +++- net/ipv6/xfrm6_policy.c | 4 +++- net/netfilter/nf_conntrack_standalone.c | 12 +++++++++--- 11 files changed, 39 insertions(+), 13 deletions(-)