| Message ID | 20240123002814.1396804-27-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | overflow: Refactor open-coded arithmetic wrap-around | expand |
* Kees Cook <keescook@chromium.org> [240122 19:36]: > 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 <geert@linux-m68k.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liam Howlett <liam.howlett@oracle.com> > Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> > Cc: Hugh Dickins <hughd@google.com> > Cc: linux-m68k@lists.linux-m68k.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > arch/m68k/kernel/sys_m68k.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/m68k/kernel/sys_m68k.c b/arch/m68k/kernel/sys_m68k.c > index 1af5e6082467..b2b9248f2566 100644 > --- 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; > struct vm_area_struct *vma; > > /* Check for overflow. */ With your nice self-documenting code, you can probably drop that comment. With or without the change, Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> > - 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) > goto out_unlock; > } > > -- > 2.34.1 >
Hi Kees, On Tue, Jan 23, 2024 at 1:35 AM Kees Cook <keescook@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 <geert@linux-m68k.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liam Howlett <liam.howlett@oracle.com> > Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> > Cc: Hugh Dickins <hughd@google.com> > Cc: linux-m68k@lists.linux-m68k.org > Signed-off-by: Kees Cook <keescook@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? > goto out_unlock; > } With the above fixed: Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> If you want me to take this through the m68k tree (for v6.9), please let me know. Thanks! Gr{oetje,eeting}s, Geert
Hi, On 23.1.2024 10.13, Geert Uytterhoeven wrote: > On Tue, Jan 23, 2024 at 1:35 AM Kees Cook <keescook@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 <geert@linux-m68k.org> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Liam Howlett <liam.howlett@oracle.com> >> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> >> Cc: Hugh Dickins <hughd@google.com> >> Cc: linux-m68k@lists.linux-m68k.org >> Signed-off-by: Kees Cook <keescook@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. - Eero >> goto out_unlock; >> } > > With the above fixed: > > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> > > If you want me to take this through the m68k tree (for v6.9), please > let me know. > Thanks! > > Gr{oetje,eeting}s, > > Geert >
Hi Eero, On Tue, Jan 23, 2024 at 2:30 PM Eero Tamminen <oak@helsinkinet.fi> wrote: > On 23.1.2024 10.13, Geert Uytterhoeven wrote: > > On Tue, Jan 23, 2024 at 1:35 AM Kees Cook <keescook@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 <geert@linux-m68k.org> > >> Cc: Andrew Morton <akpm@linux-foundation.org> > >> Cc: Arnd Bergmann <arnd@arndb.de> > >> Cc: Liam Howlett <liam.howlett@oracle.com> > >> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> > >> Cc: Hugh Dickins <hughd@google.com> > >> Cc: linux-m68k@lists.linux-m68k.org > >> Signed-off-by: Kees Cook <keescook@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
diff --git a/arch/m68k/kernel/sys_m68k.c b/arch/m68k/kernel/sys_m68k.c index 1af5e6082467..b2b9248f2566 100644 --- 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; struct vm_area_struct *vma; /* Check for overflow. */ - 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) goto out_unlock; }
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 <geert@linux-m68k.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liam Howlett <liam.howlett@oracle.com> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org> Cc: Hugh Dickins <hughd@google.com> Cc: linux-m68k@lists.linux-m68k.org Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/m68k/kernel/sys_m68k.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)