| Message ID | 20250224095826.16458-3-nicolas.bouchinet@clip-os.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fixes multiple sysctl bound checks | expand |
On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote: > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > > Bound nsm_local_state sysctl writings between SYSCTL_ZERO > and SYSCTL_INT_MAX. > > The proc_handler has thus been updated to proc_dointvec_minmax. > > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > --- > fs/lockd/svc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c > index 2c8eedc6c2cc9..984ab233af8b6 100644 > --- a/fs/lockd/svc.c > +++ b/fs/lockd/svc.c > @@ -461,7 +461,9 @@ static const struct ctl_table nlm_sysctls[] = { > .data = &nsm_local_state, > .maxlen = sizeof(int), > .mode = 0644, > - .proc_handler = proc_dointvec, > + .proc_handler = proc_dointvec_minmax, > + .extra1 = SYSCTL_ZERO, > + .extra2 = SYSCTL_INT_MAX, > }, > }; > Hi Nicolas - nsm_local_state is an unsigned 32-bit integer. The type of that value is defined by spec, because this value is exchanged between peers on the network. Perhaps this patch should replace proc_dointvec with proc_douintvec instead.
On 2/24/25 15:38, Chuck Lever wrote: > On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote: >> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> >> >> Bound nsm_local_state sysctl writings between SYSCTL_ZERO >> and SYSCTL_INT_MAX. >> >> The proc_handler has thus been updated to proc_dointvec_minmax. >> >> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> >> --- >> fs/lockd/svc.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c >> index 2c8eedc6c2cc9..984ab233af8b6 100644 >> --- a/fs/lockd/svc.c >> +++ b/fs/lockd/svc.c >> @@ -461,7 +461,9 @@ static const struct ctl_table nlm_sysctls[] = { >> .data = &nsm_local_state, >> .maxlen = sizeof(int), >> .mode = 0644, >> - .proc_handler = proc_dointvec, >> + .proc_handler = proc_dointvec_minmax, >> + .extra1 = SYSCTL_ZERO, >> + .extra2 = SYSCTL_INT_MAX, >> }, >> }; >> > Hi Nicolas - > > nsm_local_state is an unsigned 32-bit integer. The type of that value is > defined by spec, because this value is exchanged between peers on the > network. > > Perhaps this patch should replace proc_dointvec with proc_douintvec > instead. > > Hi Chuck, Thank's for your review. If `nsm_local_state` should be set to the full range of an uint32_t by a user writing in the sysctl, then yes it should use `proc_douintvec` instead of limiting it to SYSCTL_INT_MAX value (INT_MAX). I've used `proc_dointvec_minmax` since it already used `proc_dointvec` and thus was already capped at INT_MAX. Best regards, Nicolas
On Mon, Feb 24, 2025 at 09:38:17AM -0500, Chuck Lever wrote: > On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote: > > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > > > > Bound nsm_local_state sysctl writings between SYSCTL_ZERO > > and SYSCTL_INT_MAX. > > > > The proc_handler has thus been updated to proc_dointvec_minmax. > > > > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > > --- > > fs/lockd/svc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c > > index 2c8eedc6c2cc9..984ab233af8b6 100644 > > --- a/fs/lockd/svc.c > > +++ b/fs/lockd/svc.c > > @@ -461,7 +461,9 @@ static const struct ctl_table nlm_sysctls[] = { > > .data = &nsm_local_state, > > .maxlen = sizeof(int), > > .mode = 0644, > > - .proc_handler = proc_dointvec, > > + .proc_handler = proc_dointvec_minmax, > > + .extra1 = SYSCTL_ZERO, > > + .extra2 = SYSCTL_INT_MAX, > > }, > > }; > > > > Hi Nicolas - > > nsm_local_state is an unsigned 32-bit integer. The type of that value is > defined by spec, because this value is exchanged between peers on the > network. > > Perhaps this patch should replace proc_dointvec with proc_douintvec > instead. As Nicolas stated, that is completely up to how you used the variable. Things to notice: 1. If you want the full range of a unsigned long, then you should stop using proc_dointvec as it will upper limit the value to INT_MAX. 2. If you want to keep using nsm_local_state as unsigned int, then please add SYSCTL_ZERO as a lower bound to avoid assigning negative values 3. Having SYSCTL_INT_MAX is not necessary as it is already capped by proc_dointvec{_minmax,}, but it is nice to have as it makes explicit what is happening. Let me know if you take this through your trees so I can remove it from sysctl. Reviewed-by: Joel Granados <joel.granados@kernel.org> Best > > > -- > Chuck Lever
On 3/3/25 9:12 AM, Joel Granados wrote: > On Mon, Feb 24, 2025 at 09:38:17AM -0500, Chuck Lever wrote: >> On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote: >>> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> >>> >>> Bound nsm_local_state sysctl writings between SYSCTL_ZERO >>> and SYSCTL_INT_MAX. >>> >>> The proc_handler has thus been updated to proc_dointvec_minmax. >>> >>> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> >>> --- >>> fs/lockd/svc.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c >>> index 2c8eedc6c2cc9..984ab233af8b6 100644 >>> --- a/fs/lockd/svc.c >>> +++ b/fs/lockd/svc.c >>> @@ -461,7 +461,9 @@ static const struct ctl_table nlm_sysctls[] = { >>> .data = &nsm_local_state, >>> .maxlen = sizeof(int), >>> .mode = 0644, >>> - .proc_handler = proc_dointvec, >>> + .proc_handler = proc_dointvec_minmax, >>> + .extra1 = SYSCTL_ZERO, >>> + .extra2 = SYSCTL_INT_MAX, >>> }, >>> }; >>> >> >> Hi Nicolas - >> >> nsm_local_state is an unsigned 32-bit integer. The type of that value is >> defined by spec, because this value is exchanged between peers on the >> network. >> >> Perhaps this patch should replace proc_dointvec with proc_douintvec >> instead. > As Nicolas stated, that is completely up to how you used the variable. > > Things to notice: > 1. If you want the full range of a unsigned long, then you should stop > using proc_dointvec as it will upper limit the value to INT_MAX. > 2. If you want to keep using nsm_local_state as unsigned int, then > please add SYSCTL_ZERO as a lower bound to avoid assigning negative > values > 3. Having SYSCTL_INT_MAX is not necessary as it is already capped by > proc_dointvec{_minmax,}, but it is nice to have as it makes explicit > what is happening. > > Let me know if you take this through your trees so I can remove it from > sysctl. > > Reviewed-by: Joel Granados <joel.granados@kernel.org> The variable "nsm_local_state" is declared as "u32". The range of the value of nsm_local_state is supposed to be 0 - UINT_MAX (ie, an unsigned 32-bit integer), so I'm proposing: .data = &nsm_local_state, .maxlen = sizeof(int), .mode = 0644, - .proc_handler = proc_dointvec, + .proc_handler = proc_douintvec, }, }; But I suspect that the "maxlen" field should be changed to something like "sizeof(nsm_local_state)". Does that seem correct to you? I can take this patch through the NFSD tree so it can get some NFS- specific testing.
From: Chuck Lever <chuck.lever@oracle.com> On Mon, 24 Feb 2025 10:58:17 +0100, nicolas.bouchinet@clip-os.org wrote: > Bound nsm_local_state sysctl writings between SYSCTL_ZERO > and SYSCTL_INT_MAX. > > The proc_handler has thus been updated to proc_dointvec_minmax. > > Applied to nfsd-testing with modifications, thanks! [2/6] sysctl: Fixes nsm_local_state bounds commit: 8e6d33ea0159b39d670b7986324bd6135ee9d5f7 -- Chuck Lever
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c index 2c8eedc6c2cc9..984ab233af8b6 100644 --- a/fs/lockd/svc.c +++ b/fs/lockd/svc.c @@ -461,7 +461,9 @@ static const struct ctl_table nlm_sysctls[] = { .data = &nsm_local_state, .maxlen = sizeof(int), .mode = 0644, - .proc_handler = proc_dointvec, + .proc_handler = proc_dointvec_minmax, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_INT_MAX, }, };