| Message ID | 1465526452-30567-1-git-send-email-subashab@codeaurora.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 06/10/2016 04:40 AM, Subash Abhinov Kasiviswanathan wrote: > We have scripts which write to certain fields on 3.18 kernels but > this seems to be failing on 4.4 kernels. > An entry which we write to here is xfrm_aevent_rseqth which is u32. > > echo 4294967295 > /proc/sys/net/core/xfrm_aevent_rseqth > > Commit 230633d109e35b0a24277498e773edeb79b4a331 ("kernel/sysctl.c: > detect overflows when converting to int") prevented writing to > sysctl entries when integer overflow occurs. > However, this does not apply to unsigned integers. > > u32 should be able to hold 4294967295 here, however it fails due > to this check. > > static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > ... > if (*lvalp > (unsigned long) INT_MAX) > return -EINVAL; > The task of the routine is to move data from unsigned long *lvalp to int *valv considering the sign in bool *negp. So the output was never meant to be > INT_MAX. > I would like to know if introducing a new handler proc_douintvec > would work here. Sample output and implementation below. This can be > cleaned up and added for other u32 fields in kernel. The suggested change would extend the usable range of positive numbers by one bit only. As many systems are 64 bit this does not seem forward looking. I would prefer to have a routine that can handle 64 bit integers with limits (let's call it proc_doint64vec_minmax) which uses fields extra1 and extra2 of ctl_table as min and max. Then set xfrm_table[].extra1 = 0 and xfrm_table[].extra2 = UINT_MAX if you need a result in the u32 range. Best regards Heinrich Schuchardt
On 06/10/2016 04:40 AM, Subash Abhinov Kasiviswanathan wrote: > We have scripts which write to certain fields on 3.18 kernels but > this seems to be failing on 4.4 kernels. > An entry which we write to here is xfrm_aevent_rseqth which is u32. > > echo 4294967295 > /proc/sys/net/core/xfrm_aevent_rseqth You could use echo -1 > /proc/sys/net/core/xfrm_aevent_rseq as a workaround before a correction is implemented. Best regards Heinrich > > Commit 230633d109e35b0a24277498e773edeb79b4a331 ("kernel/sysctl.c: > detect overflows when converting to int") prevented writing to > sysctl entries when integer overflow occurs. > However, this does not apply to unsigned integers. > > u32 should be able to hold 4294967295 here, however it fails due > to this check. > > static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > ... > if (*lvalp > (unsigned long) INT_MAX) > return -EINVAL; > > I would like to know if introducing a new handler proc_douintvec > would work here. Sample output and implementation below. This can be > cleaned up and added for other u32 fields in kernel. > > dev0# cat /proc/sys/net/core/xfrm_aevent_rseqth > 2 > dev0# echo 4294967295 > /proc/sys/net/core/xfrm_aevent_rseqth > dev0# cat /proc/sys/net/core/xfrm_aevent_rseqth > 4294967295 > dev0# > dev0# echo -1 > /proc/sys/net/core/xfrm_aevent_rseqth > bash: echo: write error: Invalid argument > > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> > --- > include/linux/sysctl.h | 2 ++ > kernel/sysctl.c | 32 ++++++++++++++++++++++++++++++++ > net/xfrm/xfrm_sysctl.c | 2 +- > 3 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index fa7bc29..ef17db6c 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -41,6 +41,8 @@ extern int proc_dostring(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > extern int proc_dointvec(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > +extern int proc_douintvec(struct ctl_table *, int, > + void __user *, size_t *, loff_t *); > extern int proc_dointvec_minmax(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > extern int proc_dointvec_jiffies(struct ctl_table *, int, > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 725587f..6362859 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2094,6 +2094,21 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > return 0; > } > > +static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp, > + int *valp, > + int write, void *data) > +{ > + if (write) { > + if (*negp) > + return -EINVAL; > + *valp = *lvalp; > + } else { > + unsigned int val = *valp; > + *lvalp = (unsigned long)val; > + } > + return 0; > +} > + > static const char proc_wspace_sep[] = { ' ', '\t', '\n' }; > > static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, > @@ -2217,6 +2232,16 @@ int proc_dointvec(struct ctl_table *table, int write, > NULL,NULL); > } > > +/** > + * proc_douintvec - read a vector of unsigned integers > + */ > +int proc_douintvec(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + return do_proc_dointvec(table,write,buffer,lenp,ppos, > + do_proc_douintvec_conv, NULL); > +} > + > /* > * Taint values can only be increased > * This means we can safely use a temporary. > @@ -2812,6 +2837,12 @@ int proc_dointvec(struct ctl_table *table, int write, > return -ENOSYS; > } > > +int proc_douintvec(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + return -ENOSYS; > +} > + > int proc_dointvec_minmax(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos) > { > @@ -2857,6 +2888,7 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write, > * exception granted :-) > */ > EXPORT_SYMBOL(proc_dointvec); > +EXPORT_SYMBOL(proc_douintvec); > EXPORT_SYMBOL(proc_dointvec_jiffies); > EXPORT_SYMBOL(proc_dointvec_minmax); > EXPORT_SYMBOL(proc_dointvec_userhz_jiffies); > diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c > index 05a6e3d..1fa3b1a 100644 > --- a/net/xfrm/xfrm_sysctl.c > +++ b/net/xfrm/xfrm_sysctl.c > @@ -23,7 +23,7 @@ static struct ctl_table xfrm_table[] = { > .procname = "xfrm_aevent_rseqth", > .maxlen = sizeof(u32), > .mode = 0644, > - .proc_handler = proc_dointvec > + .proc_handler = proc_douintvec > }, > { > .procname = "xfrm_larval_drop", >
> The suggested change would extend the usable range of positive numbers > by one bit only. As many systems are 64 bit this does not seem forward > looking. > > I would prefer to have a routine that can handle 64 bit integers with > limits (let's call it proc_doint64vec_minmax) which uses fields extra1 > and extra2 of ctl_table as min and max. > > Then set xfrm_table[].extra1 = 0 and xfrm_table[].extra2 = UINT_MAX if > you need a result in the u32 range. > Thanks Heinrich. Do you think we can use proc_doulongvec_minmax for this?
On 2016-06-12 20:30, subashab@codeaurora.org wrote: >> The suggested change would extend the usable range of positive numbers >> by one bit only. As many systems are 64 bit this does not seem forward >> looking. >> >> I would prefer to have a routine that can handle 64 bit integers with >> limits (let's call it proc_doint64vec_minmax) which uses fields extra1 >> and extra2 of ctl_table as min and max. >> >> Then set xfrm_table[].extra1 = 0 and xfrm_table[].extra2 = UINT_MAX if >> you need a result in the u32 range. >> > > Thanks Heinrich. Do you think we can use proc_doulongvec_minmax for > this? Actually proc_doulongvec_minmax does not work here. I would expect similar problems due to casting if we use u64 (proc_doint64vec_minmax) here. static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write, { unsigned long *i, *min, *max; int vleft, first = 1, err = 0; i = (unsigned long *) data; //This cast is causing to read beyond the size of data (u32) min = (unsigned long *) table->extra1; max = (unsigned long *) table->extra2; vleft = table->maxlen / sizeof(unsigned long); //vleft is 0 because maxlen is sizeof(u32) which is lesser than sizeof(unsigned long) on x86_64. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index fa7bc29..ef17db6c 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -41,6 +41,8 @@ extern int proc_dostring(struct ctl_table *, int, void __user *, size_t *, loff_t *); extern int proc_dointvec(struct ctl_table *, int, void __user *, size_t *, loff_t *); +extern int proc_douintvec(struct ctl_table *, int, + void __user *, size_t *, loff_t *); extern int proc_dointvec_minmax(struct ctl_table *, int, void __user *, size_t *, loff_t *); extern int proc_dointvec_jiffies(struct ctl_table *, int, diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 725587f..6362859 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2094,6 +2094,21 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, return 0; } +static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp, + int *valp, + int write, void *data) +{ + if (write) { + if (*negp) + return -EINVAL; + *valp = *lvalp; + } else { + unsigned int val = *valp; + *lvalp = (unsigned long)val; + } + return 0; +} + static const char proc_wspace_sep[] = { ' ', '\t', '\n' }; static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, @@ -2217,6 +2232,16 @@ int proc_dointvec(struct ctl_table *table, int write, NULL,NULL); } +/** + * proc_douintvec - read a vector of unsigned integers + */ +int proc_douintvec(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + return do_proc_dointvec(table,write,buffer,lenp,ppos, + do_proc_douintvec_conv, NULL); +} + /* * Taint values can only be increased * This means we can safely use a temporary. @@ -2812,6 +2837,12 @@ int proc_dointvec(struct ctl_table *table, int write, return -ENOSYS; } +int proc_douintvec(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + return -ENOSYS; +} + int proc_dointvec_minmax(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { @@ -2857,6 +2888,7 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write, * exception granted :-) */ EXPORT_SYMBOL(proc_dointvec); +EXPORT_SYMBOL(proc_douintvec); EXPORT_SYMBOL(proc_dointvec_jiffies); EXPORT_SYMBOL(proc_dointvec_minmax); EXPORT_SYMBOL(proc_dointvec_userhz_jiffies); diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c index 05a6e3d..1fa3b1a 100644 --- a/net/xfrm/xfrm_sysctl.c +++ b/net/xfrm/xfrm_sysctl.c @@ -23,7 +23,7 @@ static struct ctl_table xfrm_table[] = { .procname = "xfrm_aevent_rseqth", .maxlen = sizeof(u32), .mode = 0644, - .proc_handler = proc_dointvec + .proc_handler = proc_douintvec }, { .procname = "xfrm_larval_drop",
We have scripts which write to certain fields on 3.18 kernels but this seems to be failing on 4.4 kernels. An entry which we write to here is xfrm_aevent_rseqth which is u32. echo 4294967295 > /proc/sys/net/core/xfrm_aevent_rseqth Commit 230633d109e35b0a24277498e773edeb79b4a331 ("kernel/sysctl.c: detect overflows when converting to int") prevented writing to sysctl entries when integer overflow occurs. However, this does not apply to unsigned integers. u32 should be able to hold 4294967295 here, however it fails due to this check. static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, ... if (*lvalp > (unsigned long) INT_MAX) return -EINVAL; I would like to know if introducing a new handler proc_douintvec would work here. Sample output and implementation below. This can be cleaned up and added for other u32 fields in kernel. dev0# cat /proc/sys/net/core/xfrm_aevent_rseqth 2 dev0# echo 4294967295 > /proc/sys/net/core/xfrm_aevent_rseqth dev0# cat /proc/sys/net/core/xfrm_aevent_rseqth 4294967295 dev0# dev0# echo -1 > /proc/sys/net/core/xfrm_aevent_rseqth bash: echo: write error: Invalid argument Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> --- include/linux/sysctl.h | 2 ++ kernel/sysctl.c | 32 ++++++++++++++++++++++++++++++++ net/xfrm/xfrm_sysctl.c | 2 +- 3 files changed, 35 insertions(+), 1 deletion(-)