| 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
| ||
|
Message-ID: <371c6f09-2bba-a9d6-18e8-114bea97a18d@amd.com>
Date: Wed, 14 Jul 2021 12:43:25 +0200
From: Christian König <christian.koenig@....com>
To: guangming.cao@...iatek.com
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dma-buf: add kernel count for dma_buf
Am 14.07.21 um 11:44 schrieb guangming.cao@...iatek.com:
> From: Guangming Cao <Guangming.Cao@...iatek.com>
>
> On Wed, 2021-07-14 at 10:46 +0200, Christian König wrote:
>> Am 14.07.21 um 09:11 schrieb guangming.cao@...iatek.com:
>>> From: Guangming Cao <Guangming.Cao@...iatek.com>
>>>
>>> Add a refcount for kernel to prevent UAF(Use After Free) issue.
>> Well NAK on so many levels.
>>
>>> We can assume a case like below:
>>> 1. kernel space alloc dma_buf(file count = 1)
>>> 2. kernel use dma_buf to get fd(file count = 1)
>>> 3. userspace use fd to do mapping (file count = 2)
>> Creating an userspace mapping increases the reference count for the
>> underlying file object.
>>
>> See the implementation of mmap_region():
>> ...
>> vma->vm_file = get_file(file);
>> error = call_mmap(file, vma);
>> ...
>>
>> What can happen is the the underlying exporter redirects the mmap to
>> a
>> different file, e.g. TTM or GEM drivers do that all the time.
>>
>> But this is fine since then the VA mapping is independent of the DMA-
>> buf.
>>
>>> 4. kernel call dma_buf_put (file count = 1)
>>> 5. userpsace close buffer fd(file count = 0)
>>> 6. at this time, buffer is released, but va is valid!!
>>> So we still can read/write buffer via mmap va,
>>> it maybe cause memory leak, or kernel exception.
>>> And also, if we use "ls -ll" to watch corresponding process
>>> fd link info, it also will cause kernel exception.
>>>
>>> Another case:
>>> Using dma_buf_fd to generate more than 1 fd, because
>>> dma_buf_fd will not increase file count, thus, when close
>>> the second fd, it maybe occurs error.
>> Each opened fd will increase the reference count so this is
>> certainly
>> not correct what you describe here.
>>
>> Regards,
>> Christian.
>>
> Yes, mmap will increase file count by calling get_file, so step[2] ->
> step[3], file count increase 1.
>
> But, dma_buf_fd() will not increase file count.
> function "dma_buf_fd(struct dma_buf *dmabuf, int flags)" just get an
> unused fd, via call "get_unused_fd_flags(flags)", and call
> "fd_install(fd, dmabuf->file)", it will let associated "struct file*"
> in task's fdt->fd[fd] points to this dma_buf.file, not increase the
> file count of dma_buf.file.
> I think this is confusing, I can get more than 1 fds via dma_buf_fd,
> but they don't need to close it because they don't increase file count.
>
> However, dma_buf_put() can decrease file count at kernel side directly.
> If somebody write a ko to put file count of dma_buf.file many times, it
> will cause buffer freed earlier than except. At last on Android, I
> think this is a little bit dangerous.
dma_buf_fd() takes the dma_buf pointer and converts it into a fd. So the
reference is consumed.
That's why users of this interface make sure to get a separate
reference, see drm_gem_prime_handle_to_fd() for example:
...
out_have_handle:
ret = dma_buf_fd(dmabuf, flags);
/*
* We must _not_ remove the buffer from the handle cache since the
newly
* created dma buf is already linked in the global obj->dma_buf
pointer,
* and that is invariant as long as a userspace gem handle exists.
* Closing the handle will clean out the cache anyway, so we don't
leak.
*/
if (ret < 0) {
goto fail_put_dmabuf;
} else {
*prime_fd = ret;
ret = 0;
}
goto out;
fail_put_dmabuf:
dma_buf_put(dmabuf);
out:
...
You could submit a patch to improve the documentation and explicitly
note on dma_buf_fd() that the reference is consumed, but all of this is
working perfectly fine.
Regards,
Christian.
>
>>> Solution:
>>> Add a kernel count for dma_buf, and make sure the file count
>>> of dma_buf.file hold by kernel is 1.
>>>
>>> Notes: For this solution, kref couldn't work because kernel ref
>>> maybe added from 0, but kref don't allow it.
>>>
>>> Signed-off-by: Guangming Cao <Guangming.Cao@...iatek.com>
>>> ---
>>> drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++----
>>> include/linux/dma-buf.h | 6 ++++--
>>> 2 files changed, 23 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 511fe0d217a0..04ee92aac8b9 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -62,6 +62,7 @@ static void dma_buf_release(struct dentry
>>> *dentry)
>>> if (unlikely(!dmabuf))
>>> return;
>>>
>>> + WARN_ON(atomic64_read(&dmabuf->kernel_ref));
>>> BUG_ON(dmabuf->vmapping_counter);
>>>
>>> /*
>>> @@ -555,6 +556,7 @@ struct dma_buf *dma_buf_export(const struct
>>> dma_buf_export_info *exp_info)
>>> goto err_module;
>>> }
>>>
>>> + atomic64_set(&dmabuf->kernel_ref, 1);
>>> dmabuf->priv = exp_info->priv;
>>> dmabuf->ops = exp_info->ops;
>>> dmabuf->size = exp_info->size;
>>> @@ -617,6 +619,9 @@ int dma_buf_fd(struct dma_buf *dmabuf, int
>>> flags)
>>>
>>> fd_install(fd, dmabuf->file);
>>>
>>> + /* Add file cnt for each new fd */
>>> + get_file(dmabuf->file);
>>> +
>>> return fd;
>>> }
>>> EXPORT_SYMBOL_GPL(dma_buf_fd);
>>> @@ -626,12 +631,13 @@ EXPORT_SYMBOL_GPL(dma_buf_fd);
>>> * @fd: [in] fd associated with the struct dma_buf to be
>>> returned
>>> *
>>> * On success, returns the struct dma_buf associated with an fd;
>>> uses
>>> - * file's refcounting done by fget to increase refcount. returns
>>> ERR_PTR
>>> - * otherwise.
>>> + * dmabuf's ref refcounting done by kref_get to increase refcount.
>>> + * Returns ERR_PTR otherwise.
>>> */
>>> struct dma_buf *dma_buf_get(int fd)
>>> {
>>> struct file *file;
>>> + struct dma_buf *dmabuf;
>>>
>>> file = fget(fd);
>>>
>>> @@ -643,7 +649,12 @@ struct dma_buf *dma_buf_get(int fd)
>>> return ERR_PTR(-EINVAL);
>>> }
>>>
>>> - return file->private_data;
>>> + dmabuf = file->private_data;
>>> + /* replace file count increase as ref increase for kernel user
>>> */
>>> + get_dma_buf(dmabuf);
>>> + fput(file);
>>> +
>>> + return dmabuf;
>>> }
>>> EXPORT_SYMBOL_GPL(dma_buf_get);
>>>
>>> @@ -662,7 +673,11 @@ void dma_buf_put(struct dma_buf *dmabuf)
>>> if (WARN_ON(!dmabuf || !dmabuf->file))
>>> return;
>>>
>>> - fput(dmabuf->file);
>>> + if (WARN_ON(!atomic64_read(&dmabuf->kernel_ref)))
>>> + return;
>>> +
>>> + if (!atomic64_dec_return(&dmabuf->kernel_ref))
>>> + fput(dmabuf->file);
>>> }
>>> EXPORT_SYMBOL_GPL(dma_buf_put);
>>>
>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>>> index efdc56b9d95f..bc790cb028eb 100644
>>> --- a/include/linux/dma-buf.h
>>> +++ b/include/linux/dma-buf.h
>>> @@ -308,6 +308,7 @@ struct dma_buf_ops {
>>> struct dma_buf {
>>> size_t size;
>>> struct file *file;
>>> + atomic64_t kernel_ref;
>>> struct list_head attachments;
>>> const struct dma_buf_ops *ops;
>>> struct mutex lock;
>>> @@ -436,7 +437,7 @@ struct dma_buf_export_info {
>>> .owner = THIS_MODULE }
>>>
>>> /**
>>> - * get_dma_buf - convenience wrapper for get_file.
>>> + * get_dma_buf - increase a kernel ref of dma-buf
>>> * @dmabuf: [in] pointer to dma_buf
>>> *
>>> * Increments the reference count on the dma-buf, needed in case
>>> of drivers
>>> @@ -446,7 +447,8 @@ struct dma_buf_export_info {
>>> */
>>> static inline void get_dma_buf(struct dma_buf *dmabuf)
>>> {
>>> - get_file(dmabuf->file);
>>> + if (atomic64_inc_return(&dmabuf->kernel_ref) == 1)
>>> + get_file(dmabuf->file);
>>> }
>>>
>>> /**
Powered by blists - more mailing lists