Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1401802 > unrolled thread
| Started by | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| First post | 2016-05-17 01:00 +0200 |
| Last post | 2016-05-17 23:40 +0200 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v7 1/3] create SMAF module Emil Velikov <emil.l.velikov@gmail.com> - 2016-05-17 01:00 +0200
Re: [PATCH v7 1/3] create SMAF module Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2016-05-17 16:00 +0200
Re: [PATCH v7 1/3] create SMAF module Daniel Vetter <daniel@ffwll.ch> - 2016-05-17 17:50 +0200
Re: [PATCH v7 1/3] create SMAF module Emil Velikov <emil.l.velikov@gmail.com> - 2016-05-17 21:10 +0200
Re: [PATCH v7 1/3] create SMAF module Emil Velikov <emil.l.velikov@gmail.com> - 2016-05-17 23:40 +0200
Re: [PATCH v7 1/3] create SMAF module Daniel Vetter <daniel@ffwll.ch> - 2016-05-17 23:40 +0200
| From | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| Date | 2016-05-17 01:00 +0200 |
| Subject | Re: [PATCH v7 1/3] create SMAF module |
| Message-ID | <rzzEu-4H3-9@gated-at.bofh.it> |
Hi Benjamin,
I'd suspect you're interested in some feedback on these, so here is a few :-)
Sadly (ideally?) nothing serious, but a bunch minor suggestions, plus
the odd bug.
On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> --- /dev/null
> +++ b/drivers/smaf/smaf-core.c
> @@ -0,0 +1,794 @@
> +/*
> + * smaf.c
The comment does not match the actual file name.
You could give a brief summary of the file(s), if you're feeling gracious ;-)
> +
> +/**
> + * smaf_grant_access - return true if the specified device can get access
> + * to the memory area
> + *
Reading this makes me wonder if {request,allow}_access won't be better name ?
> +static int smaf_secure_handle(struct smaf_handle *handle)
> +{
> + if (atomic_read(&handle->is_secure))
> + return 0;
> +
> + if (!have_secure_module())
> + return -EINVAL;
> +
> + handle->secure_ctx = smaf_dev.secure->create_ctx();
> +
Should one use a temporary variable so that the caller provided
storage is unchanged in case of an error ?
> + if (!handle->secure_ctx)
> + return -EINVAL;
> +
> + atomic_set(&handle->is_secure, 1);
> + return 0;
> +}
> +
> +int smaf_register_secure(struct smaf_secure *s)
> +{
> + /* make sure that secure module have all required functions
> + * to avoid test them each time later
> + */
> + WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
> + !s->grant_access || !s->revoke_access);
> +
Is something like below reasonable thing to do in the kernel ?
Same question goes for smaf_register_allocator() further down.
if (!s || ....)
return -ESHOULDNEVERHAPPEN;
> +static struct vm_operations_struct smaf_vma_ops = {
Ops/vfucs normally are const data. Is there something preventing us here ?
> + .close = smaf_vm_close,
> +};
> +
> +static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> + struct smaf_handle *handle = dmabuf->priv;
> + bool ret;
> + enum dma_data_direction dir;
> +
> + /* if no allocator attached, get the first allocator */
> + if (!handle->allocator) {
> + struct smaf_allocator *alloc;
> +
> + mutex_lock(&smaf_dev.lock);
> + alloc = smaf_get_first_allocator(dmabuf);
> + mutex_unlock(&smaf_dev.lock);
> +
> + /* still no allocator ? */
> + if (!alloc)
> + return -EINVAL;
> +
> + handle->allocator = alloc;
> + }
> +
> + if (!handle->db_alloc) {
> + struct dma_buf *db_alloc;
> +
> + db_alloc = handle->allocator->allocate(dmabuf,
> + handle->length,
> + handle->flags);
> + if (!db_alloc)
> + return -EINVAL;
> +
> + handle->db_alloc = db_alloc;
> + }
> +
The above half of the function looks identical to smaf_map_dma_buf().
Worth factoring it out to a helper function ?
> +static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
> + struct dma_buf_attachment *attach)
> +{
> + struct smaf_handle *handle = dmabuf->priv;
> + struct dma_buf_attachment *db_attach;
> +
> + if (!handle->db_alloc)
> + return 0;
> +
Shouldn't one return an error (-EINVAL or similar) here ?
> +static struct dma_buf_ops smaf_dma_buf_ops = {
const ? From a very quick look the compiler should warn us about it -
"smaf_dma_buf_ops discards const qualifier" or similar.
> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
> +{
> + struct smaf_handle *handle;
> +
> + DEFINE_DMA_BUF_EXPORT_INFO(info);
> +
> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle)
> + return ERR_PTR(-ENOMEM);
> +
Err this should be return NULL; correct ?
> + info.ops = &smaf_dma_buf_ops;
> + info.size = round_up(length, PAGE_SIZE);
> + info.flags = flags;
> + info.priv = handle;
> +
> + handle->dmabuf = dma_buf_export(&info);
> + if (IS_ERR(handle->dmabuf)) {
> + kfree(handle);
> + return NULL;
> + }
> +
> + handle->length = info.size;
> + handle->flags = flags;
> +
> + return handle;
> +}
> +EXPORT_SYMBOL(smaf_create_handle);
> +
> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + switch (cmd) {
> + case SMAF_IOC_CREATE:
> + {
> + struct smaf_create_data data;
> + struct smaf_handle *handle;
> +
> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> + return -EFAULT;
> +
> + handle = smaf_create_handle(data.length, data.flags);
We want to sanitise the input data.{length,flags} before sending it
deeper in the kernel.
> + if (!handle)
> + return -EINVAL;
> +
> + if (data.name[0]) {
> + /* user force allocator selection */
> + if (smaf_select_allocator_by_name(handle->dmabuf,
> + data.name)) {
> + dma_buf_put(handle->dmabuf);
Missing free(handle), here and through the rest of the case statement ?
> + return -EINVAL;
> + }
> + }
> +
> + handle->fd = dma_buf_fd(handle->dmabuf, data.flags);
> + if (handle->fd < 0) {
Worth adding smaf_DEselect_allocator_by_name() and using it here + below ?
> + dma_buf_put(handle->dmabuf);
> + return -EINVAL;
> + }
> +
> + data.fd = handle->fd;
> + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
> + dma_buf_put(handle->dmabuf);
> + return -EFAULT;
> + }
> + break;
> + }
> + case SMAF_IOC_GET_SECURE_FLAG:
> + {
> + struct smaf_secure_flag data;
> + struct dma_buf *dmabuf;
> +
> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> + return -EFAULT;
> +
> + dmabuf = dma_buf_get(data.fd);
Worth adding if (data.fd == -1) return -EINVAL; ?
> +
> +static const struct file_operations smaf_fops = {
> + .owner = THIS_MODULE,
There was a recent 'crusade' to get rid of these. Are you sure we
want/need this ?
> + .unlocked_ioctl = smaf_ioctl,
> +};
> +
> +static int __init smaf_init(void)
> +{
> + int ret = 0;
> +
Please drop the default initialization.
> + smaf_dev.misc_dev.minor = MISC_DYNAMIC_MINOR;
> + smaf_dev.misc_dev.name = "smaf";
> + smaf_dev.misc_dev.fops = &smaf_fops;
> +
Initialize the global static variable (smaf_dev) upon declaration ?
> --- /dev/null
> +++ b/include/linux/smaf-secure.h
> +/**
> + * smaf_create_handle - create a smaf_handle with the give length and flags
> + * do not allocate memory but provide smaf_handle->dmabuf that can be
> + * shared between devices.
> + *
> + * @length: buffer size
> + * @flags: handle flags
> + */
> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags);
> +
Inspired by the bug (?) in this function I think you want to document
the return value throughout the header.
> +#endif
> diff --git a/include/uapi/linux/smaf.h b/include/uapi/linux/smaf.h
> new file mode 100644
> index 0000000..5a9201b
> --- /dev/null
> +++ b/include/uapi/linux/smaf.h
> @@ -0,0 +1,66 @@
> +/*
> + * smaf.h
> + *
Would be nice if we had more elaborate comment in an UAPI header.
> +/**
> + * struct smaf_create_data - allocation parameters
> + * @length: size of the allocation
> + * @flags: flags passed to allocator
> + * @name: name of the allocator to be selected, could be NULL
Is it guaranteed to be null terminated ? If so one should mentioned it
otherwise your userspace should be fixed.
Same comments apply for smaf_info::name.
> + * @fd: returned file descriptor
> + */
> +struct smaf_create_data {
> + size_t length;
> + unsigned int flags;
> + char name[ALLOCATOR_NAME_LENGTH];
> + int fd;
The structs here feels quite fragile. Please read up on Daniel
Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
useful is such process.
Hopefully I haven't lost the plot with the above, if I had don't be
shy to point out.
Thanks,
Emil
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
[toc] | [next] | [standalone]
| From | Benjamin Gaignard <benjamin.gaignard@linaro.org> |
|---|---|
| Date | 2016-05-17 16:00 +0200 |
| Message-ID | <rzNHw-5fA-17@gated-at.bofh.it> |
| In reply to | #1401802 |
Hello Emil,
thanks for your review.
I have understand most of your remarks and I'm fixing them
but some points aren't obvious for me...
2016-05-17 0:58 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
> Hi Benjamin,
>
> I'd suspect you're interested in some feedback on these, so here is a few :-)
> Sadly (ideally?) nothing serious, but a bunch minor suggestions, plus
> the odd bug.
>
> On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
>
>> --- /dev/null
>> +++ b/drivers/smaf/smaf-core.c
>> @@ -0,0 +1,794 @@
>> +/*
>> + * smaf.c
> The comment does not match the actual file name.
>
> You could give a brief summary of the file(s), if you're feeling gracious ;-)
>
>
>> +
>> +/**
>> + * smaf_grant_access - return true if the specified device can get access
>> + * to the memory area
>> + *
> Reading this makes me wonder if {request,allow}_access won't be better name ?
>
grant and revoke sound more secure oriented for me but that could change
>
>> +static int smaf_secure_handle(struct smaf_handle *handle)
>> +{
>> + if (atomic_read(&handle->is_secure))
>> + return 0;
>> +
>> + if (!have_secure_module())
>> + return -EINVAL;
>> +
>> + handle->secure_ctx = smaf_dev.secure->create_ctx();
>> +
> Should one use a temporary variable so that the caller provided
> storage is unchanged in case of an error ?
>
>> + if (!handle->secure_ctx)
>> + return -EINVAL;
>> +
>> + atomic_set(&handle->is_secure, 1);
>> + return 0;
>> +}
>> +
>
>
>> +int smaf_register_secure(struct smaf_secure *s)
>> +{
>> + /* make sure that secure module have all required functions
>> + * to avoid test them each time later
>> + */
>> + WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
>> + !s->grant_access || !s->revoke_access);
>> +
> Is something like below reasonable thing to do in the kernel ?
> Same question goes for smaf_register_allocator() further down.
>
> if (!s || ....)
> return -ESHOULDNEVERHAPPEN;
>
>
>
>> +static struct vm_operations_struct smaf_vma_ops = {
> Ops/vfucs normally are const data. Is there something preventing us here ?
>
>> + .close = smaf_vm_close,
>> +};
>> +
>> +static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>> +{
>> + struct smaf_handle *handle = dmabuf->priv;
>> + bool ret;
>> + enum dma_data_direction dir;
>> +
>> + /* if no allocator attached, get the first allocator */
>> + if (!handle->allocator) {
>> + struct smaf_allocator *alloc;
>> +
>> + mutex_lock(&smaf_dev.lock);
>> + alloc = smaf_get_first_allocator(dmabuf);
>> + mutex_unlock(&smaf_dev.lock);
>> +
>> + /* still no allocator ? */
>> + if (!alloc)
>> + return -EINVAL;
>> +
>> + handle->allocator = alloc;
>> + }
>> +
>> + if (!handle->db_alloc) {
>> + struct dma_buf *db_alloc;
>> +
>> + db_alloc = handle->allocator->allocate(dmabuf,
>> + handle->length,
>> + handle->flags);
>> + if (!db_alloc)
>> + return -EINVAL;
>> +
>> + handle->db_alloc = db_alloc;
>> + }
>> +
> The above half of the function looks identical to smaf_map_dma_buf().
> Worth factoring it out to a helper function ?
>
>
>> +static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
>> + struct dma_buf_attachment *attach)
>> +{
>> + struct smaf_handle *handle = dmabuf->priv;
>> + struct dma_buf_attachment *db_attach;
>> +
>> + if (!handle->db_alloc)
>> + return 0;
>> +
> Shouldn't one return an error (-EINVAL or similar) here ?
No because a device could attach itself on the buffer and the
allocator will only
be selected at the first map_attach call.
The goal is to delay the allocation until all devices are attached to
select the best allocator.
>
>
>> +static struct dma_buf_ops smaf_dma_buf_ops = {
> const ? From a very quick look the compiler should warn us about it -
> "smaf_dma_buf_ops discards const qualifier" or similar.
>
>
>> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
>> +{
>> + struct smaf_handle *handle;
>> +
>> + DEFINE_DMA_BUF_EXPORT_INFO(info);
>> +
>> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>> + if (!handle)
>> + return ERR_PTR(-ENOMEM);
>> +
> Err this should be return NULL; correct ?
>
>> + info.ops = &smaf_dma_buf_ops;
>> + info.size = round_up(length, PAGE_SIZE);
>> + info.flags = flags;
>> + info.priv = handle;
>> +
>> + handle->dmabuf = dma_buf_export(&info);
>> + if (IS_ERR(handle->dmabuf)) {
>> + kfree(handle);
>> + return NULL;
>> + }
>> +
>> + handle->length = info.size;
>> + handle->flags = flags;
>> +
>> + return handle;
>> +}
>> +EXPORT_SYMBOL(smaf_create_handle);
>> +
>> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> +{
>> + switch (cmd) {
>> + case SMAF_IOC_CREATE:
>> + {
>> + struct smaf_create_data data;
>> + struct smaf_handle *handle;
>> +
>> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>> + return -EFAULT;
>> +
>> + handle = smaf_create_handle(data.length, data.flags);
> We want to sanitise the input data.{length,flags} before sending it
> deeper in the kernel.
Sorry but can you elaborate little more here ?
I don't understand your expectations.
>
>> + if (!handle)
>> + return -EINVAL;
>> +
>> + if (data.name[0]) {
>> + /* user force allocator selection */
>> + if (smaf_select_allocator_by_name(handle->dmabuf,
>> + data.name)) {
>> + dma_buf_put(handle->dmabuf);
> Missing free(handle), here and through the rest of the case statement ?
>
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + handle->fd = dma_buf_fd(handle->dmabuf, data.flags);
>> + if (handle->fd < 0) {
> Worth adding smaf_DEselect_allocator_by_name() and using it here + below ?
>
>> + dma_buf_put(handle->dmabuf);
>> + return -EINVAL;
>> + }
>> +
>> + data.fd = handle->fd;
>> + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
>> + dma_buf_put(handle->dmabuf);
>> + return -EFAULT;
>> + }
>> + break;
>> + }
>> + case SMAF_IOC_GET_SECURE_FLAG:
>> + {
>> + struct smaf_secure_flag data;
>> + struct dma_buf *dmabuf;
>> +
>> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>> + return -EFAULT;
>> +
>> + dmabuf = dma_buf_get(data.fd);
> Worth adding if (data.fd == -1) return -EINVAL; ?
>
>
>
>> +
>> +static const struct file_operations smaf_fops = {
>> + .owner = THIS_MODULE,
> There was a recent 'crusade' to get rid of these. Are you sure we
> want/need this ?
>
>> + .unlocked_ioctl = smaf_ioctl,
>> +};
>> +
>> +static int __init smaf_init(void)
>> +{
>> + int ret = 0;
>> +
> Please drop the default initialization.
>
>> + smaf_dev.misc_dev.minor = MISC_DYNAMIC_MINOR;
>> + smaf_dev.misc_dev.name = "smaf";
>> + smaf_dev.misc_dev.fops = &smaf_fops;
>> +
> Initialize the global static variable (smaf_dev) upon declaration ?
>
>
>> --- /dev/null
>> +++ b/include/linux/smaf-secure.h
>
>> +/**
>> + * smaf_create_handle - create a smaf_handle with the give length and flags
>> + * do not allocate memory but provide smaf_handle->dmabuf that can be
>> + * shared between devices.
>> + *
>> + * @length: buffer size
>> + * @flags: handle flags
>> + */
>> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags);
>> +
> Inspired by the bug (?) in this function I think you want to document
> the return value throughout the header.
It is useless the add this function in this .h file I will remove it
and fix the comment in structure defintion
>
>> +#endif
>> diff --git a/include/uapi/linux/smaf.h b/include/uapi/linux/smaf.h
>> new file mode 100644
>> index 0000000..5a9201b
>> --- /dev/null
>> +++ b/include/uapi/linux/smaf.h
>> @@ -0,0 +1,66 @@
>> +/*
>> + * smaf.h
>> + *
> Would be nice if we had more elaborate comment in an UAPI header.
>
>
>> +/**
>> + * struct smaf_create_data - allocation parameters
>> + * @length: size of the allocation
>> + * @flags: flags passed to allocator
>> + * @name: name of the allocator to be selected, could be NULL
> Is it guaranteed to be null terminated ? If so one should mentioned it
> otherwise your userspace should be fixed.
> Same comments apply for smaf_info::name.
I have used strncpy everywhere to avoid this problem but maybe it is not enough
>
>
>> + * @fd: returned file descriptor
>> + */
>> +struct smaf_create_data {
>> + size_t length;
>> + unsigned int flags;
>> + char name[ALLOCATOR_NAME_LENGTH];
>> + int fd;
> The structs here feels quite fragile. Please read up on Daniel
> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> useful is such process.
>
if I describe the structures like this:
/**
* struct smaf_create_data - allocation parameters
* @length: size of the allocation
* @flags: flags passed to allocator
* @name_len: length of name
* @name: name of the allocator to be selected, could be NULL
* @fd: returned file descriptor
*/
struct smaf_create_data {
size_t length;
unsigned int flags;
size_t name_len;
char __user *name;
int fd;
char padding[44];
};
does it sound more robust for you ?
> Hopefully I haven't lost the plot with the above, if I had don't be
> shy to point out.
>
> Thanks,
> Emil
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
--
Benjamin Gaignard
Graphic Working Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-17 17:50 +0200 |
| Message-ID | <rzPpU-6oq-5@gated-at.bofh.it> |
| In reply to | #1402358 |
On Tue, May 17, 2016 at 03:50:41PM +0200, Benjamin Gaignard wrote:
> 2016-05-17 0:58 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
> > On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> >> + * @fd: returned file descriptor
> >> + */
> >> +struct smaf_create_data {
> >> + size_t length;
> >> + unsigned int flags;
> >> + char name[ALLOCATOR_NAME_LENGTH];
> >> + int fd;
> > The structs here feels quite fragile. Please read up on Daniel
> > Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> > useful is such process.
> >
> if I describe the structures like this:
> /**
> * struct smaf_create_data - allocation parameters
> * @length: size of the allocation
> * @flags: flags passed to allocator
> * @name_len: length of name
> * @name: name of the allocator to be selected, could be NULL
> * @fd: returned file descriptor
> */
> struct smaf_create_data {
> size_t length;
> unsigned int flags;
> size_t name_len;
> char __user *name;
> int fd;
> char padding[44];
> };
>
> does it sound more robust for you ?
>
> > Hopefully I haven't lost the plot with the above, if I had don't be
> > shy to point out.
> >
> > Thanks,
> > Emil
> >
> > [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
Please read this doc in it's entirety, ask on irc if you don't get certain
parts. Then come back an rework your patch.
Super short summary: _All_ the types you've used are absolute no-go in
ioctl structs.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| Date | 2016-05-17 21:10 +0200 |
| Message-ID | <rzSxt-7q-29@gated-at.bofh.it> |
| In reply to | #1402358 |
On 17 May 2016 at 14:50, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> Hello Emil,
>
> thanks for your review.
> I have understand most of your remarks and I'm fixing them
> but some points aren't obvious for me...
>
Sure thing. Thanks for being honest.
>
> No because a device could attach itself on the buffer and the
> allocator will only
> be selected at the first map_attach call.
> The goal is to delay the allocation until all devices are attached to
> select the best allocator.
>
I see. Makes sense.
>>> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>> +{
>>> + switch (cmd) {
>>> + case SMAF_IOC_CREATE:
>>> + {
>>> + struct smaf_create_data data;
>>> + struct smaf_handle *handle;
>>> +
>>> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>>> + return -EFAULT;
>>> +
>>> + handle = smaf_create_handle(data.length, data.flags);
>> We want to sanitise the input data.{length,flags} before sending it
>> deeper in the kernel.
>
> Sorry but can you elaborate little more here ?
> I don't understand your expectations.
>
You want to determine which flags are 'considered useful' at this
stage, and reject anything else. As-is you inject any flags that the
user gives you directly into the 'guts' of the kernel. This is not
good from security and future expandability POV.
About the length you want a similar thing. size_t is unsigned (great),
although ideally you'll want to check/determine if one cannot exploit
it, integer overflow being the more common suspect. This may be quite
hard to track, so I'd stick with the flags checking at least.
> It is useless the add this function in this .h file I will remove it
> and fix the comment in structure defintion
I've seen both approaches - description next to the declaration or
definition. I'd rather not pick sides, as people might throw rotten
fruit at me ;-)
>>> +/**
>>> + * struct smaf_create_data - allocation parameters
>>> + * @length: size of the allocation
>>> + * @flags: flags passed to allocator
>>> + * @name: name of the allocator to be selected, could be NULL
Just occurred to - you might want to comment what the user should
expect if NULL. Any at random one will be used or otherwise. Very
quick description on the heuristics used might be good as well.
>> Is it guaranteed to be null terminated ? If so one should mentioned it
>> otherwise your userspace should be fixed.
>> Same comments apply for smaf_info::name.
>
> I have used strncpy everywhere to avoid this problem but maybe it is not enough
>
According to the man page
"The strncpy() function is similar, except that at most n bytes of src
are copied. Warning: If there is no null byte among the first n bytes
of src, the string placed in dest will _not_ be null-terminated."
Annotation is mine obviously. I believe that after the strncpy 'name'
is/was assumed (used as) a NULL terminated string.
>>
>>
>>> + * @fd: returned file descriptor
>>> + */
>>> +struct smaf_create_data {
>>> + size_t length;
>>> + unsigned int flags;
>>> + char name[ALLOCATOR_NAME_LENGTH];
>>> + int fd;
>> The structs here feels quite fragile. Please read up on Daniel
>> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
>> useful is such process.
>>
> if I describe the structures like this:
> /**
> * struct smaf_create_data - allocation parameters
> * @length: size of the allocation
> * @flags: flags passed to allocator
> * @name_len: length of name
> * @name: name of the allocator to be selected, could be NULL
> * @fd: returned file descriptor
> */
> struct smaf_create_data {
> size_t length;
> unsigned int flags;
> size_t name_len;
> char __user *name;
> int fd;
> char padding[44];
> };
>
> does it sound more robust for you ?
>
Seems like you changed 'name' from fixed size array to char *. Which
actually gets us slightly further away from robust.
As Daniel said, please read through the hole file.
Here is a (slightly incomplete) gist of it all:
- you want to to use __[us]{8,16,32,64} and __kernel_size_t types everywhere
- each member of the struct must be the same offset for both 32bit and
64bit builds. ^^ helps with that
- double check for gaps in the struct - I think you have a few
- each struct should have it's old 'flags' which you'll use to
indicate the capability of the ioctl and thus the size of struct used.
i.e. it's for future use.
Obviously the other two structs need similar polish.
Here is how you can check things with pahole:
- Create a simple C file that includes the header and has an instance
of each struct - it doesn't have to be use any of them.
- Compile for 32 and 64 bit with -g -O0. Compare the struct layout -
it should be identical in both cases.
Hope that makes things a bit clearer.
Emil
[toc] | [prev] | [next] | [standalone]
| From | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| Date | 2016-05-17 23:40 +0200 |
| Message-ID | <rzUSB-1tG-1@gated-at.bofh.it> |
| In reply to | #1402538 |
On 17 May 2016 at 22:29, Daniel Vetter <daniel@ffwll.ch> wrote: > Please don't use __kernel_size_t, it's only for backwards compat if you > already botched an ioctl definition ;-) > That explains why I've not seen it in (m)any other UAPI headers but our legacy ones. Thank you ! Emil
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-05-17 23:40 +0200 |
| Message-ID | <rzUSB-1tG-3@gated-at.bofh.it> |
| In reply to | #1402538 |
On Tue, May 17, 2016 at 08:04:59PM +0100, Emil Velikov wrote:
> On 17 May 2016 at 14:50, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> > Hello Emil,
> >
> > thanks for your review.
> > I have understand most of your remarks and I'm fixing them
> > but some points aren't obvious for me...
> >
> Sure thing. Thanks for being honest.
>
> >
> > No because a device could attach itself on the buffer and the
> > allocator will only
> > be selected at the first map_attach call.
> > The goal is to delay the allocation until all devices are attached to
> > select the best allocator.
> >
> I see. Makes sense.
>
>
> >>> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >>> +{
> >>> + switch (cmd) {
> >>> + case SMAF_IOC_CREATE:
> >>> + {
> >>> + struct smaf_create_data data;
> >>> + struct smaf_handle *handle;
> >>> +
> >>> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> >>> + return -EFAULT;
> >>> +
> >>> + handle = smaf_create_handle(data.length, data.flags);
> >> We want to sanitise the input data.{length,flags} before sending it
> >> deeper in the kernel.
> >
> > Sorry but can you elaborate little more here ?
> > I don't understand your expectations.
> >
> You want to determine which flags are 'considered useful' at this
> stage, and reject anything else. As-is you inject any flags that the
> user gives you directly into the 'guts' of the kernel. This is not
> good from security and future expandability POV.
>
> About the length you want a similar thing. size_t is unsigned (great),
> although ideally you'll want to check/determine if one cannot exploit
> it, integer overflow being the more common suspect. This may be quite
> hard to track, so I'd stick with the flags checking at least.
>
>
> > It is useless the add this function in this .h file I will remove it
> > and fix the comment in structure defintion
>
> I've seen both approaches - description next to the declaration or
> definition. I'd rather not pick sides, as people might throw rotten
> fruit at me ;-)
>
>
> >>> +/**
> >>> + * struct smaf_create_data - allocation parameters
> >>> + * @length: size of the allocation
> >>> + * @flags: flags passed to allocator
> >>> + * @name: name of the allocator to be selected, could be NULL
> Just occurred to - you might want to comment what the user should
> expect if NULL. Any at random one will be used or otherwise. Very
> quick description on the heuristics used might be good as well.
>
> >> Is it guaranteed to be null terminated ? If so one should mentioned it
> >> otherwise your userspace should be fixed.
> >> Same comments apply for smaf_info::name.
> >
> > I have used strncpy everywhere to avoid this problem but maybe it is not enough
> >
> According to the man page
>
> "The strncpy() function is similar, except that at most n bytes of src
> are copied. Warning: If there is no null byte among the first n bytes
> of src, the string placed in dest will _not_ be null-terminated."
>
> Annotation is mine obviously. I believe that after the strncpy 'name'
> is/was assumed (used as) a NULL terminated string.
>
> >>
> >>
> >>> + * @fd: returned file descriptor
> >>> + */
> >>> +struct smaf_create_data {
> >>> + size_t length;
> >>> + unsigned int flags;
> >>> + char name[ALLOCATOR_NAME_LENGTH];
> >>> + int fd;
> >> The structs here feels quite fragile. Please read up on Daniel
> >> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> >> useful is such process.
> >>
> > if I describe the structures like this:
> > /**
> > * struct smaf_create_data - allocation parameters
> > * @length: size of the allocation
> > * @flags: flags passed to allocator
> > * @name_len: length of name
> > * @name: name of the allocator to be selected, could be NULL
> > * @fd: returned file descriptor
> > */
> > struct smaf_create_data {
> > size_t length;
> > unsigned int flags;
> > size_t name_len;
> > char __user *name;
> > int fd;
> > char padding[44];
> > };
> >
> > does it sound more robust for you ?
> >
> Seems like you changed 'name' from fixed size array to char *. Which
> actually gets us slightly further away from robust.
>
> As Daniel said, please read through the hole file.
>
> Here is a (slightly incomplete) gist of it all:
> - you want to to use __[us]{8,16,32,64} and __kernel_size_t types everywhere
Please don't use __kernel_size_t, it's only for backwards compat if you
already botched an ioctl definition ;-)
Also I recomened you only use 32/64bit sized types, otherwise aligning
stuff gets more complicated. Maybe u8 for strings.
Also you must align everything to its own size explicitly by adding
padding fields. And if you have a 64 bit value anywhere, you must align
the entire struct to 64bits too, otherwise just align to 32bits.
This is a bit more than what's required, but it's much harder to screw up
that way.
-Daniel
> - each member of the struct must be the same offset for both 32bit and
> 64bit builds. ^^ helps with that
> - double check for gaps in the struct - I think you have a few
> - each struct should have it's old 'flags' which you'll use to
> indicate the capability of the ioctl and thus the size of struct used.
> i.e. it's for future use.
>
> Obviously the other two structs need similar polish.
>
> Here is how you can check things with pahole:
> - Create a simple C file that includes the header and has an instance
> of each struct - it doesn't have to be use any of them.
> - Compile for 32 and 64 bit with -g -O0. Compare the struct layout -
> it should be identical in both cases.
>
> Hope that makes things a bit clearer.
>
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web