| Message ID | 20240123002814.1396804-30-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | overflow: Refactor open-coded arithmetic wrap-around | expand |
On Mon, Jan 22, 2024 at 04:27:05PM -0800, Kees Cook wrote: Hi Kees, ... > arch/s390/include/asm/stacktrace.h | 6 ++++-- > arch/s390/kernel/machine_kexec_file.c | 5 +++-- Subject does not match. These need to be two separate commits. > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/s390/include/asm/stacktrace.h b/arch/s390/include/asm/stacktrace.h > index 31ec4f545e03..3ce08d32a8ad 100644 > --- a/arch/s390/include/asm/stacktrace.h > +++ b/arch/s390/include/asm/stacktrace.h > @@ -34,11 +34,13 @@ int get_stack_info(unsigned long sp, struct task_struct *task, > static inline bool on_stack(struct stack_info *info, > unsigned long addr, size_t len) > { > + unsigned long sum; > + > if (info->type == STACK_TYPE_UNKNOWN) > return false; > - if (addr + len < addr) > + if (check_add_overflow(addr, len, &sum)) Why not add_would_overflow()? > return false; > - return addr >= info->begin && addr + len <= info->end; > + return addr >= info->begin && sum <= info->end; > } > > /* > diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c > index 8d207b82d9fe..e5e925423061 100644 > --- a/arch/s390/kernel/machine_kexec_file.c > +++ b/arch/s390/kernel/machine_kexec_file.c > @@ -238,6 +238,7 @@ void *kexec_file_add_components(struct kimage *image, > unsigned long max_command_line_size = LEGACY_COMMAND_LINE_SIZE; > struct s390_load_data data = {0}; > unsigned long minsize; > + unsigned long sum; Please, use min_kernel_buf_len instead of sum. @Sven, could you please correct me if (minsize + max_command_line_size) means something else. > int ret; > > data.report = ipl_report_init(&ipl_block); > @@ -256,10 +257,10 @@ void *kexec_file_add_components(struct kimage *image, > if (data.parm->max_command_line_size) > max_command_line_size = data.parm->max_command_line_size; > > - if (minsize + max_command_line_size < minsize) > + if (check_add_overflow(minsize, max_command_line_size, &sum)) > goto out; > > - if (image->kernel_buf_len < minsize + max_command_line_size) > + if (image->kernel_buf_len < sum) > goto out; > > if (image->cmdline_buf_len >= max_command_line_size) Thanks!
Alexander Gordeev <agordeev@linux.ibm.com> writes: > On Mon, Jan 22, 2024 at 04:27:05PM -0800, Kees Cook wrote: >> diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c >> index 8d207b82d9fe..e5e925423061 100644 >> --- a/arch/s390/kernel/machine_kexec_file.c >> +++ b/arch/s390/kernel/machine_kexec_file.c >> @@ -238,6 +238,7 @@ void *kexec_file_add_components(struct kimage *image, >> unsigned long max_command_line_size = LEGACY_COMMAND_LINE_SIZE; >> struct s390_load_data data = {0}; >> unsigned long minsize; >> + unsigned long sum; > > Please, use min_kernel_buf_len instead of sum. > > @Sven, could you please correct me if (minsize + max_command_line_size) > means something else. Your understanding is correct, minsize + max_command_line_size is the minimum required size of the kernel image.
diff --git a/arch/s390/include/asm/stacktrace.h b/arch/s390/include/asm/stacktrace.h index 31ec4f545e03..3ce08d32a8ad 100644 --- a/arch/s390/include/asm/stacktrace.h +++ b/arch/s390/include/asm/stacktrace.h @@ -34,11 +34,13 @@ int get_stack_info(unsigned long sp, struct task_struct *task, static inline bool on_stack(struct stack_info *info, unsigned long addr, size_t len) { + unsigned long sum; + if (info->type == STACK_TYPE_UNKNOWN) return false; - if (addr + len < addr) + if (check_add_overflow(addr, len, &sum)) return false; - return addr >= info->begin && addr + len <= info->end; + return addr >= info->begin && sum <= info->end; } /* diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c index 8d207b82d9fe..e5e925423061 100644 --- a/arch/s390/kernel/machine_kexec_file.c +++ b/arch/s390/kernel/machine_kexec_file.c @@ -238,6 +238,7 @@ void *kexec_file_add_components(struct kimage *image, unsigned long max_command_line_size = LEGACY_COMMAND_LINE_SIZE; struct s390_load_data data = {0}; unsigned long minsize; + unsigned long sum; int ret; data.report = ipl_report_init(&ipl_block); @@ -256,10 +257,10 @@ void *kexec_file_add_components(struct kimage *image, if (data.parm->max_command_line_size) max_command_line_size = data.parm->max_command_line_size; - if (minsize + max_command_line_size < minsize) + if (check_add_overflow(minsize, max_command_line_size, &sum)) goto out; - if (image->kernel_buf_len < minsize + max_command_line_size) + if (image->kernel_buf_len < sum) goto out; if (image->cmdline_buf_len >= max_command_line_size)
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: Heiko Carstens <hca@linux.ibm.com> Cc: Vasily Gorbik <gor@linux.ibm.com> Cc: Alexander Gordeev <agordeev@linux.ibm.com> Cc: Christian Borntraeger <borntraeger@linux.ibm.com> Cc: Sven Schnelle <svens@linux.ibm.com> Cc: Nico Boehr <nrb@linux.ibm.com> Cc: Philipp Rudo <prudo@redhat.com> Cc: Baoquan He <bhe@redhat.com> Cc: Tao Liu <ltao@redhat.com> Cc: Alexander Egorenkov <egorenar@linux.ibm.com> Cc: linux-s390@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/s390/include/asm/stacktrace.h | 6 ++++-- arch/s390/kernel/machine_kexec_file.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)