lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Za-K-3aSBULf4NWE@FVFF77S0Q05N>
Date: Tue, 23 Jan 2024 09:46:35 +0000
From: Mark Rutland <mark.rutland@....com>
To: Kees Cook <keescook@...omium.org>
Cc: linux-hardening@...r.kernel.org,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/82] overflow: Refactor open-coded arithmetic
 wrap-around

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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ