Hi Eero, On Tue, Jan 23, 2024 at 2:30 PM Eero Tamminen <o...@helsinkinet.fi> wrote: > On 23.1.2024 10.13, Geert Uytterhoeven wrote: > > On Tue, Jan 23, 2024 at 1:35 AM Kees Cook <keesc...@chromium.org> wrote: > >> In an effort to separate intentional arithmetic wrap-around from > >> unexpected wrap-around, we need to refactor places that depend on this > >> kind of math. One of the most common code patterns of this is: > >> > >> VAR + value < VAR > >> > >> Notably, this is considered "undefined behavior" for signed and pointer > >> types, which the kernel works around by using the -fno-strict-overflow > >> option in the build[1] (which used to just be -fwrapv). Regardless, we > >> want to get the kernel source to the position where we can meaningfully > >> instrument arithmetic wrap-around conditions and catch them when they > >> are unexpected, regardless of whether they are signed[2], unsigned[3], > >> or pointer[4] types. > >> > >> Refactor open-coded unsigned wrap-around addition test to use > >> check_add_overflow(), retaining the result for later usage (which removes > >> the redundant open-coded addition). This paves the way to enabling the > >> unsigned wrap-around sanitizer[2] in the future. > >> > >> Link: > >> https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1] > >> Link: https://github.com/KSPP/linux/issues/26 [2] > >> Link: https://github.com/KSPP/linux/issues/27 [3] > >> Link: https://github.com/KSPP/linux/issues/344 [4] > >> Cc: Geert Uytterhoeven <ge...@linux-m68k.org> > >> Cc: Andrew Morton <a...@linux-foundation.org> > >> Cc: Arnd Bergmann <a...@arndb.de> > >> Cc: Liam Howlett <liam.howl...@oracle.com> > >> Cc: "Matthew Wilcox (Oracle)" <wi...@infradead.org> > >> Cc: Hugh Dickins <hu...@google.com> > >> Cc: linux-m...@lists.linux-m68k.org > >> Signed-off-by: Kees Cook <keesc...@chromium.org> > > > > Thanks for your patch! > > > >> --- a/arch/m68k/kernel/sys_m68k.c > >> +++ b/arch/m68k/kernel/sys_m68k.c > >> @@ -391,10 +391,11 @@ sys_cacheflush (unsigned long addr, int scope, int > >> cache, unsigned long len) > >> > >> mmap_read_lock(current->mm); > >> } else { > >> + unsigned long sum; > > > > "sum" sounds like this is a dummy variable, to please the third > > parameter of check_add_overflow()... > > > >> struct vm_area_struct *vma; > >> > >> /* Check for overflow. */ > > > > I agree with Liam: please drop the comment. > > > >> - if (addr + len < addr) > >> + if (check_add_overflow(addr, len, &sum)) > >> goto out; > >> > >> /* > >> @@ -403,7 +404,7 @@ sys_cacheflush (unsigned long addr, int scope, int > >> cache, unsigned long len) > >> */ > >> mmap_read_lock(current->mm); > >> vma = vma_lookup(current->mm, addr); > >> - if (!vma || addr + len > vma->vm_end) > >> + if (!vma || sum > vma->vm_end) > > > > ... Oh, it is actually used. What about renaming it to "end" instead? > > IMHO this is more descriptive: > + if (check_add_overflow(addr, len, &sum)) > > than this: > + if (check_add_overflow(addr, len, &end)) > > "sum" is IMHO quite obviously sum of the preceding args, whereas I do > not know what "end" would be.
"end" is the end of the block of size "len" pointed to by "addr". IMHO "if (sum > vma->vm_end)" is less descriptive... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds