linux-kernel - [BUG] AMDKFD: criu_checkpoint() error path treats userspace pointer as kernel pointer

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-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez3a_35R48khgW8D1MxokEqmYP3mBsUwhMq5Vfi5BiyYCQ@mail.gmail.com>
Date:   Mon, 31 Oct 2022 15:20:56 +0100
From:   Jann Horn <jannh@...gle.com>
To:     Rajneesh Bhardwaj <rajneesh.bhardwaj@....com>,
        Felix Kuehling <felix.kuehling@....com>
Cc:     David Yat Sin <david.yatsin@....com>,
        Alex Deucher <alexander.deucher@....com>,
        kernel list <linux-kernel@...r.kernel.org>,
        amd-gfx@...ts.freedesktop.org, "Pan, Xinhui" <Xinhui.Pan@....com>,
        Christian König <christian.koenig@....com>
Subject: [BUG] AMDKFD: criu_checkpoint() error path treats userspace pointer
 as kernel pointer

be072b06c73970 ("drm/amdkfd: CRIU export BOs as prime dmabuf objects")
added an error path in criu_checkpoint() that (unless I'm completely
misreading this) treats the userspace-supplied args->bos (which was
previously used as a userspace pointer when passed to
criu_checkpoint_bos()) as a kernel pointer:

  ret = criu_checkpoint_bos(p, num_bos, (uint8_t __user *)args->bos,
      (uint8_t __user *)args->priv_data, &priv_offset);
  if (ret)
    goto exit_unlock;
  [...]
close_bo_fds:
  if (ret) {
    /* If IOCTL returns err, user assumes all FDs opened in
criu_dump_bos are closed */
    uint32_t i;
    struct kfd_criu_bo_bucket *bo_buckets = (struct kfd_criu_bo_bucket
*) args->bos;

    for (i = 0; i < num_bos; i++) {
      if (bo_buckets[i].alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM)
        close_fd(bo_buckets[i].dmabuf_fd);
    }
  }

This seems very wrong, and also like it's guaranteed to blow up as
soon as it runs on a machine with SMAP, which makes me think that this
codepath was probably never exercised?

(Also note that just changing this to copy_from_user() instead would
still be wrong, because malicious/bogus userspace could change the FD
number to the KFD device's FD, and the VFS assumes that an FD can't be
closed while it's being accessed in a single-threaded process.)