On Sat, Jul 11, 2015 at 10:35 AM, Robert Xiao <b...@cs.cmu.edu> wrote: > On LP64 systems, reading a sysctl file containing an INT_MIN (-2147483648) > could incorrectly show -18446744071562067968 due to an incorrect conversion > in do_proc_dointvec_conv. This patch fixes the edge case by converting to > unsigned int first to avoid sign extending INT_MIN to unsigned long. > > Test: > > root:/proc/sys/kernel# echo -2147483648 0 0 0 > printk > root:/proc/sys/kernel# cat printk > > Without patch, produces -18446744071562067968 0 0 0. > With patch, should produce -2147483648 0 0 0. > > Signed-off-by: Robert Xiao <b...@cs.cmu.edu> > --- > kernel/sysctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 19b62b5..464df36 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -1995,10 +1995,10 @@ static int do_proc_dointvec_conv(bool *negp, unsigned > long *lvalp, > int val = *valp; > if (val < 0) { > *negp = true; > - *lvalp = (unsigned long)-val; > + *lvalp = (unsigned int)-val; > } else { > *negp = false; > - *lvalp = (unsigned long)val; > + *lvalp = (unsigned int)val; > } > } > return 0;
I don't know why am I CC'ed on this - CC'ing Andrew along with Eric and Kees who seem to have worked directly on sysctl.c not too long ago. That said, I took a look at this and I think this patch is wrong. Casting to unsigned int instead of unsigned long *after* the negation is bogus, because we have if (val < 0) ... *lvalp = (unsigned long)-val; and the compiler is free to assume -val to be positive and use the sign-extend instruction. On gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7) that I have here the cast to unsigned int works only with -O1, with -O2 it goes to town and uses cltq which sign-extends: neg %eax movb $0x1,(%rdi) cltq IMO the right way to do this would be to first cast to unsigned long and then negate - that way we will first sign-extend and then negate an unsigned, which is well defined. Also, this needs to be done not just for do_proc_dointvec_conv(), but for do_proc_dointvec_minmax_conv() and jiffies functions as well (although it's probably virtually impossible to set val to exactly INT_MIN through jiffies write branches). Speaking of write branches, only do_proc_dointvec_conv() does check it's input properly, so that's something to look at. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/