| 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/ >
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. > > 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 I see that you have received several reviews suggesting that you post some of the patches in this series separately. Please remove these for your V2 so we do not duplicate efforts. Thx > > --- > > 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(-) > > -- > 2.48.1 >
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(-)