Re: [PATCH v2 7/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

On 06/06/2018 08:10 AM, Oleksandr Andrushchenko wrote:
> On 06/05/2018 01:07 AM, Boris Ostrovsky wrote:
>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:


>> +
>> +static struct sg_table *
>> +dmabuf_exp_ops_map_dma_buf(struct dma_buf_attachment *attach,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum dma_data_direction dir)
>> +{
>> +ÂÂÂ struct gntdev_dmabuf_attachment *gntdev_dmabuf_attach =
>> attach->priv;
>> +ÂÂÂ struct gntdev_dmabuf *gntdev_dmabuf = attach->dmabuf->priv;
>> +ÂÂÂ struct sg_table *sgt;
>> +
>> +ÂÂÂ pr_debug("Mapping %d pages for dev %p\n", gntdev_dmabuf->nr_pages,
>> +ÂÂÂÂÂÂÂÂ attach->dev);
>> +
>> +ÂÂÂ if (WARN_ON(dir == DMA_NONE || !gntdev_dmabuf_attach))
>>
>> WARN_ON_ONCE. Here and elsewhere.
> Why? The UAPI may be used by different applications, thus we might
> lose warnings for some of them. Having WARN_ON will show problems
> for multiple users, not for the first one.
> Does this make sense to still use WARN_ON?


Just as with pr_err call somewhere else the concern here is that
userland (which I think is where this is eventually called from?) may
intentionally trigger the error, flooding the log.

And even this is not directly called from userland there is still a
possibility of triggering this error multiple times.


>>
>>> +
>>> +ÂÂÂ if (use_ptemod) {
>>> +ÂÂÂÂÂÂÂ pr_err("Cannot provide dma-buf: use_ptemode %d\n",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ use_ptemod);
>> No pr_err here please. This can potentially become a DoS vector as it
>> comes directly from ioctl.
>>
>> I would, in fact, revisit other uses of pr_err in this file.
> Sure, all of pr_err can actually be pr_debug...

I'd check even further and see if any prink is needed. I think I saw a
couple that were not especially useful.


>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ map = dmabuf_exp_alloc_backing_storage(priv, flags, count);
>>
>> @count comes from userspace. dmabuf_exp_alloc_backing_storage only
>> checks for it to be >0. Should it be checked for some sane max value?
> This is not easy as it is hard to tell what could be that
> max value. For DMA buffers if count is too big then allocation
> will fail, so need to check for max here (dma_alloc_{xxx} will
> filter out too big allocations).

OK, that may be sufficient. BTW, I believe there were other loops with
@count being the control variable. Please see if a user can pass a bogus
value.

> For Xen balloon allocations I cannot tell what could be that
> max value neither. Tough question how to limit.

I think in balloon there is also a guarantee (of sorts) that something
prior to a loop will fail.


-boris