Received-SPF: pass (google.com: domain of linux-kernel+bounces-35035-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 23 Jan 2024 09:46:35 +0000
From: Mark Rutland <mark.rutland@arm.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/82] overflow: Refactor open-coded arithmetic
 wrap-around
Message-ID: <Za-K-3aSBULf4NWE@FVFF77S0Q05N>
References: <20240122235208.work.748-kees@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240122235208.work.748-kees@kernel.org>

On Mon, Jan 22, 2024 at 04:26:35PM -0800, Kees Cook wrote:
> Hi,

Hi Kees,

> In our continuing effort to eliminate root causes of flaws in the kernel,
> this series is the start to providing a way to have sensible coverage
> for catching unexpected arithmetic wrap-around.
> 
> A quick word on language: while discussing[1] the finer details of
> the C standard's view on arithmetic, I was disabused of using the term
> "overflow" when what I really mean is "wrap-around". When describing
> security vulnerabilities, "overflow" is the common term and often used
> interchangeably with "wrap-around". Strictly speaking, though, "overflow"
> applies only to signed[2] and pointer[3] types, and "wrap-around" is for
> unsigned[4]. An arithmetic "overflow" is considered undefined behavior,
> which has caused our builds pain in the past, since "impossible"
> conditions might get elided by the compiler. As a result, we build
> with -fno-strict-overflow which coverts all "overflow" conditions into
> "wrap-around" (i.e. 2s complement), regardless of type.
> 
> All this is to say I am discussing arithmetic wrap-around, which is
> the condition where the value exceeds a type's maximum value (or goes
> below its minimum value) and wraps around. I'm not interested in the
> narrow definition of "undefined behavior" -- we need to stamp out the
> _unexpected_ behavior, where the kernel operates on a pathological value
> that wrapped around without the code author's intent.

With that in mind, I note that this patch primarily modifies addition
operations, but leaves subtraction operations unchanged (even though those
permit the value to go below the minimum, or above the maximum if a negative
value is used as the subtrahend).

Shouldn't we address both at the same time? I'll note that in many places the
same logic is used for both the add and sub, and can legitimately overflow or
underflow; I hope that whatever we use to suppress overflow warnings also
ignores underflow.

[...]

Looking at the diffstat, I think you've missed a few places:

>  Documentation/process/deprecated.rst          | 36 ++++++++
>  arch/arc/kernel/unwind.c                      |  7 +-
>  arch/arm/nwfpe/softfloat.c                    |  2 +-
>  arch/arm64/include/asm/atomic_lse.h           |  8 +-
>  arch/arm64/include/asm/stacktrace/common.h    |  2 +-
>  arch/arm64/kvm/vgic/vgic-kvm-device.c         |  6 +-
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c            |  2 +-
>  arch/arm64/kvm/vgic/vgic-v2.c                 | 10 ++-
>  arch/m68k/kernel/sys_m68k.c                   |  5 +-
>  arch/nios2/kernel/sys_nios2.c                 |  2 +-
>  arch/powerpc/platforms/powernv/opal-prd.c     |  2 +-
>  arch/powerpc/xmon/xmon.c                      |  2 +-
>  arch/s390/include/asm/stacktrace.h            |  6 +-
>  arch/s390/kernel/machine_kexec_file.c         |  5 +-
>  arch/s390/mm/gmap.c                           |  4 +-
>  arch/s390/mm/vmem.c                           |  2 +-
>  arch/sh/kernel/sys_sh.c                       |  2 +-
>  arch/x86/include/asm/atomic.h                 |  2 +-
>  arch/x86/kernel/cpu/sgx/ioctl.c               |  6 +-
>  arch/x86/kvm/svm/sev.c                        |  5 +-
>  crypto/adiantum.c                             |  2 +-
>  drivers/acpi/custom_method.c                  |  2 +-
>  drivers/char/agp/generic.c                    |  2 +-
>  drivers/crypto/amcc/crypto4xx_alg.c           |  2 +-
>  drivers/crypto/axis/artpec6_crypto.c          |  2 +-
>  drivers/dma-buf/dma-buf.c                     |  7 +-
>  drivers/fpga/dfl.c                            |  5 +-
>  drivers/fsi/fsi-core.c                        |  6 +-
>  drivers/gpu/drm/i915/i915_vma.c               |  2 +-
>  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c |  8 +-
>  drivers/gpu/drm/vc4/vc4_validate.c            |  7 +-
>  drivers/md/dm-switch.c                        |  2 +-
>  drivers/md/dm-verity-target.c                 |  2 +-
>  drivers/md/dm-writecache.c                    |  2 +-
>  drivers/net/ethernet/sun/niu.c                |  5 +-
>  drivers/net/wireless/ath/wil6210/wmi.c        |  2 +-
>  drivers/net/wireless/marvell/mwifiex/pcie.c   |  6 +-
>  drivers/net/xen-netback/hash.c                |  2 +-
>  drivers/pci/pci.c                             |  2 +-
>  drivers/remoteproc/pru_rproc.c                |  2 +-
>  drivers/remoteproc/remoteproc_elf_loader.c    |  2 +-
>  drivers/remoteproc/remoteproc_virtio.c        |  4 +-
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c            |  2 +-
>  drivers/scsi/sd_zbc.c                         |  2 +-
>  drivers/staging/vme_user/vme.c                |  2 +-
>  drivers/vhost/vringh.c                        |  8 +-
>  drivers/virtio/virtio_pci_modern_dev.c        |  4 +-
>  fs/aio.c                                      |  2 +-
>  fs/bcachefs/bkey.c                            |  4 +-
>  fs/bcachefs/fs.c                              |  2 +-
>  fs/bcachefs/quota.c                           |  2 +-
>  fs/bcachefs/util.c                            |  2 +-
>  fs/btrfs/extent_map.c                         |  6 +-
>  fs/btrfs/extent_map.h                         |  6 +-
>  fs/btrfs/ordered-data.c                       |  2 +-
>  fs/ext4/block_validity.c                      |  2 +-
>  fs/ext4/extents.c                             |  5 +-
>  fs/ext4/resize.c                              |  2 +-
>  fs/f2fs/file.c                                |  2 +-
>  fs/f2fs/verity.c                              |  2 +-
>  fs/hpfs/alloc.c                               |  2 +-
>  fs/ntfs3/record.c                             |  4 +-
>  fs/ocfs2/resize.c                             |  2 +-
>  fs/read_write.c                               |  8 +-
>  fs/remap_range.c                              |  2 +-
>  fs/select.c                                   | 13 +--
>  fs/smb/client/readdir.c                       |  5 +-
>  fs/smb/client/smb2pdu.c                       |  4 +-
>  fs/udf/balloc.c                               |  4 +-
>  include/linux/compiler_types.h                | 29 +++++-

This misses the include/asm-generic/{atomic,atomic64}.h implementations.

This also misses the include/linux/atomic/atomic-arch-fallback.h
implementations. Those are generated from the scripts/atomic/fallbacks/*
templates, and you'll need to adjust at least fetch_add_unless and
inc_unless_negative. As noted on other patches, my preference is to use
add_wrap() in those.

>  include/linux/overflow.h                      | 76 +++++++++++++++-
>  ipc/mqueue.c                                  |  2 +-
>  ipc/shm.c                                     |  6 +-
>  kernel/bpf/verifier.c                         | 12 +--
>  kernel/time/timekeeping.c                     |  2 +-
>  lib/Kconfig.ubsan                             | 27 ++++++

This misses lib/atomic64.c.

Thanks,
Mark.