[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <425617d8-3d36-f50e-e582-826a4c6da32d@amd.com>
Date: Mon, 31 Oct 2022 13:53:55 -0400
From: Felix Kuehling <felix.kuehling@....com>
To: Jann Horn <jannh@...gle.com>,
Rajneesh Bhardwaj <rajneesh.bhardwaj@....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: Re: [BUG] AMDKFD: criu_checkpoint() error path treats userspace
pointer as kernel pointer
Am 2022-10-31 um 10:20 schrieb Jann Horn:
> 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.)
Thank you for catching this, and thank you for the advice. In other
words, we need to store a copy of the FDs in a kernel mode buffer that
is not accessibly by usermode, so we can reliably close the correct FDs
in the error handling code path. Rajneesh and I will fix this ASAP.
Do you think we should also avoid copying the FDs to usermode before
we're sure that we'll return success? I don't think it would make a big
difference because user mode could try to guess the FDs and use them
before we return from the ioctl either way.
Regards,
Felix
Powered by blists - more mailing lists