Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 02/10/2017 11:28 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx]
>> Sent: 10 February 2017 16:18
>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx
>> Cc: Juergen Gross <jgross@xxxxxxxx>
>> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>
>> On 02/10/2017 09:24 AM, Paul Durrant wrote:
>>> +static long privcmd_ioctl_dm_op(void __user *udata)
>>> +{
>>> +   struct privcmd_dm_op kdata;
>>> +   struct privcmd_dm_op_buf *kbufs;
>>> +   unsigned int nr_pages = 0;
>>> +   struct page **pages = NULL;
>>> +   struct xen_dm_op_buf *xbufs = NULL;
>>> +   unsigned int i;
>>> +   long rc;
>>> +
>>> +   if (copy_from_user(&kdata, udata, sizeof(kdata)))
>>> +           return -EFAULT;
>>> +
>>> +   if (kdata.num == 0)
>>> +           return 0;
>>> +
>>> +   /*
>>> +    * Set a tolerable upper limit on the number of buffers
>>> +    * without being overly restrictive, since we can't easily
>>> +    * predict what future dm_ops may require.
>>> +    */
>> I think this deserves its own macro since it really has nothing to do
>> with page size, has it? Especially since you are referencing it again
>> below too.
>>
>>
>>> +   if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
>>> +           return -E2BIG;
>>> +
>>> +   kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
>>> +   if (!kbufs)
>>> +           return -ENOMEM;
>>> +
>>> +   if (copy_from_user(kbufs, kdata.ubufs,
>>> +                      sizeof(*kbufs) * kdata.num)) {
>>> +           rc = -EFAULT;
>>> +           goto out;
>>> +   }
>>> +
>>> +   for (i = 0; i < kdata.num; i++) {
>>> +           if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
>>> +                          kbufs[i].size)) {
>>> +                   rc = -EFAULT;
>>> +                   goto out;
>>> +           }
>>> +
>>> +           nr_pages += DIV_ROUND_UP(
>>> +                   offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>>> +                   PAGE_SIZE);
>>> +   }
>>> +
>>> +   /*
>>> +    * Again, set a tolerable upper limit on the number of pages
>>> +    * needed to lock all the buffers without being overly
>>> +    * restrictive, since we can't easily predict the size of
>>> +    * buffers future dm_ops may use.
>>> +    */
>> OTOH, these two cases describe different types of copying (the first one
>> is for buffer descriptors and the second is for buffers themselves). And
>> so should they be limited by the same value?
>>
> I think there needs to be some limit and limiting the allocation to a page 
> was the best I came up with. Can you think of a better one?

How about something like (with rather arbitrary values)

#define PRIVCMD_DMOP_MAX_NUM_BUFFERS       16
#define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ     4096

and make them part of the interface (i.e. put them into privcmd.h)?

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel