diff mbox series

[v2,4/6] sysctl: Fixes scsi_logging_level bounds

Message ID 20250224095826.16458-5-nicolas.bouchinet@clip-os.org (mailing list archive)
State Accepted
Headers show
Series Fixes multiple sysctl bound checks | expand

Commit Message

Nicolas Bouchinet Feb. 24, 2025, 9:58 a.m. UTC 
From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Bound scsi_logging_level 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>
---
 drivers/scsi/scsi_sysctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Martin K. Petersen Feb. 25, 2025, 1:20 a.m. UTC | #1 
Hi Nicolas!

> --- a/drivers/scsi/scsi_sysctl.c
> +++ b/drivers/scsi/scsi_sysctl.c
> @@ -17,7 +17,9 @@ static const struct ctl_table scsi_table[] = {
>  	  .data		= &scsi_logging_level,
>  	  .maxlen	= sizeof(scsi_logging_level),
>  	  .mode		= 0644,
> -	  .proc_handler	= proc_dointvec },
> +	  .proc_handler	= proc_dointvec_minmax,
> +	  .extra1	= SYSCTL_ZERO,
> +	  .extra2	= SYSCTL_INT_MAX },

scsi_logging_level is a bitmask and should be unsigned.
Nicolas Bouchinet Feb. 25, 2025, 10:47 a.m. UTC | #2 
On 2/25/25 02:20, Martin K. Petersen wrote:
> Hi Nicolas!
>
>> --- a/drivers/scsi/scsi_sysctl.c
>> +++ b/drivers/scsi/scsi_sysctl.c
>> @@ -17,7 +17,9 @@ static const struct ctl_table scsi_table[] = {
>>   	  .data		= &scsi_logging_level,
>>   	  .maxlen	= sizeof(scsi_logging_level),
>>   	  .mode		= 0644,
>> -	  .proc_handler	= proc_dointvec },
>> +	  .proc_handler	= proc_dointvec_minmax,
>> +	  .extra1	= SYSCTL_ZERO,
>> +	  .extra2	= SYSCTL_INT_MAX },
> scsi_logging_level is a bitmask and should be unsigned.
>

Hi Martin,

Thank's for your review.

Does `scsi_logging_level` needs the full range of a unsigned 32-bit 
integer ?
As it was using `proc_dointvec`, it was capped to an INT_MAX.

If it effectively need the full range of an unsigned 32-bit integer, the
`proc_handler` could be changed to `proc_douintvec` as suggested by Chuck.

Best regards,

Nicolas
Joel Granados March 3, 2025, 2:04 p.m. UTC | #3 
On Tue, Feb 25, 2025 at 11:47:42AM +0100, Nicolas Bouchinet wrote:
> 
> On 2/25/25 02:20, Martin K. Petersen wrote:
> > Hi Nicolas!
> > 
> > > --- a/drivers/scsi/scsi_sysctl.c
> > > +++ b/drivers/scsi/scsi_sysctl.c
> > > @@ -17,7 +17,9 @@ static const struct ctl_table scsi_table[] = {
> > >   	  .data		= &scsi_logging_level,
> > >   	  .maxlen	= sizeof(scsi_logging_level),
> > >   	  .mode		= 0644,
> > > -	  .proc_handler	= proc_dointvec },
> > > +	  .proc_handler	= proc_dointvec_minmax,
> > > +	  .extra1	= SYSCTL_ZERO,
> > > +	  .extra2	= SYSCTL_INT_MAX },
> > scsi_logging_level is a bitmask and should be unsigned.
> > 
> 
> Hi Martin,
> 
> Thank's for your review.
> 
> Does `scsi_logging_level` needs the full range of a unsigned 32-bit integer
> ?
> As it was using `proc_dointvec`, it was capped to an INT_MAX.
> 
> If it effectively need the full range of an unsigned 32-bit integer, the
> `proc_handler` could be changed to `proc_douintvec` as suggested by Chuck.
And as mentioned in another patch in this series:
1. Having the upper bound be SYSCTL_INT_MAX is not necessary (as it is
   silently capped by proc_dointvec_minmax, but it is good to have as it
   adds to the understanding on what the range of the values are.

2. Having the lower bound capped by SYSCTL_ZERO is needed as it will
   prevent ppl trying to assigning negative values to the unsigned integer

Let me know if you take this through the scsi subsystem so I know to
drop it from sysctl 

Best

Reviewed-by: Joel Granados <joel.granados@kernel.org>


> 
> Best regards,
> 
> Nicolas
>
Martin K. Petersen March 4, 2025, 2:24 a.m. UTC | #4 
Joel,

> 1. Having the upper bound be SYSCTL_INT_MAX is not necessary (as it is
>    silently capped by proc_dointvec_minmax, but it is good to have as it
>    adds to the understanding on what the range of the values are.
>
> 2. Having the lower bound capped by SYSCTL_ZERO is needed as it will
>    prevent ppl trying to assigning negative values to the unsigned integer
>
> Let me know if you take this through the scsi subsystem so I know to
> drop it from sysctl 

Applied to 6.15/scsi-staging, thanks!
Joel Granados March 5, 2025, 2:44 p.m. UTC | #5 
On Mon, Mar 03, 2025 at 09:24:43PM -0500, Martin K. Petersen wrote:
> 
> Joel,
> 
> > 1. Having the upper bound be SYSCTL_INT_MAX is not necessary (as it is
> >    silently capped by proc_dointvec_minmax, but it is good to have as it
> >    adds to the understanding on what the range of the values are.
> >
> > 2. Having the lower bound capped by SYSCTL_ZERO is needed as it will
> >    prevent ppl trying to assigning negative values to the unsigned integer
> >
> > Let me know if you take this through the scsi subsystem so I know to
> > drop it from sysctl 
> 
> Applied to 6.15/scsi-staging, thanks!
Thx!!

@Nicolas: Please take this out of your next version as it is already
going upstream.

Best

> 
> -- 
> Martin K. Petersen	Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_sysctl.c b/drivers/scsi/scsi_sysctl.c
index be4aef0f4f996..055a03a83ad68 100644
--- a/drivers/scsi/scsi_sysctl.c
+++ b/drivers/scsi/scsi_sysctl.c
@@ -17,7 +17,9 @@  static const struct ctl_table scsi_table[] = {
 	  .data		= &scsi_logging_level,
 	  .maxlen	= sizeof(scsi_logging_level),
 	  .mode		= 0644,
-	  .proc_handler	= proc_dointvec },
+	  .proc_handler	= proc_dointvec_minmax,
+	  .extra1	= SYSCTL_ZERO,
+	  .extra2	= SYSCTL_INT_MAX },
 };
 
 static struct ctl_table_header *scsi_table_header;