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/
>
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.