Re: [PATCH v3] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

2018-05-08 Thread Hans Verkuil 
On 05/08/2018 07:35 PM, Sami Tolvanen wrote:
> This change fixes indirect call mismatches with Control-Flow Integrity
> checking, which are caused by calling standard ioctls using a function
> pointer that doesn't match the type of the actual function.

I really like the patch, but we've drifted away quite a bit from the original
patch, and I think the commit message needs to be updated a bit.

Just mention that IOCTL_INFO_STD has been removed, we now always call a function
and that DEFINE_V4L_STUB_FUNC is used to create stub functions where needed.

Basically the current message describes the problem (good), but it doesn't
describe the chosen solution (bad).

Sorry for asking for a v4, but this is core v4l2 code, so we're more careful
about this.

Regards,

Hans

> 
> Signed-off-by: Sami Tolvanen 
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
>  1 file changed, 130 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index de5d96dbe69e0..a40dbec271f1d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
>   unsigned int ioctl;
>   u32 flags;
>   const char * const name;
> - union {
> - u32 offset;
> - int (*func)(const struct v4l2_ioctl_ops *ops,
> - struct file *file, void *fh, void *p);
> - } u;
> + int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
> + void *fh, void *p);
>   void (*debug)(const void *arg, bool write_only);
>  };
>  
> @@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
>  #define INFO_FL_PRIO (1 << 0)
>  /* This control can be valid if the filehandle passes a control handler. */
>  #define INFO_FL_CTRL (1 << 1)
> -/* This is a standard ioctl, no need for special code */
> -#define INFO_FL_STD  (1 << 2)
> -/* This is ioctl has its own function */
> -#define INFO_FL_FUNC (1 << 3)
>  /* Queuing ioctl */
> -#define INFO_FL_QUEUE(1 << 4)
> +#define INFO_FL_QUEUE(1 << 2)
>  /* Always copy back result, even on error */
> -#define INFO_FL_ALWAYS_COPY  (1 << 5)
> +#define INFO_FL_ALWAYS_COPY  (1 << 3)
>  /* Zero struct from after the field to the end */
>  #define INFO_FL_CLEAR(v4l2_struct, field)\
>   ((offsetof(struct v4l2_struct, field) + \
> sizeof(((struct v4l2_struct *)0)->field)) << 16)
>  #define INFO_FL_CLEAR_MASK   (_IOC_SIZEMASK << 16)
>  
> -#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)  
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_STD,  \
> - .name = #_ioctl,\
> - .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
> - .debug = _debug,\
> +#define DEFINE_V4L_STUB_FUNC(_vidioc)\
> + static int v4l_stub_ ## _vidioc(\
> + const struct v4l2_ioctl_ops *ops,   \
> + struct file *file, void *fh, void *p)   \
> + {   \
> + return ops->vidioc_ ## _vidioc(file, fh, p);\
>   }
>  
> -#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_FUNC, \
> - .name = #_ioctl,\
> - .u.func = _func,\
> - .debug = _debug,\
> +#define IOCTL_INFO(_ioctl, _func, _debug, _flags)\
> + [_IOC_NR(_ioctl)] = {   \
> + .ioctl = _ioctl,\
> + .flags = _flags,\
> + .name = #_ioctl,\
> + .func = _func,  \
> + .debug = _debug,\
>   }
>  
> +DEFINE_V4L_STUB_FUNC(g_fbuf)
> +DEFINE_V4L_STUB_FUNC(s_fbuf)
> +DEFINE_V4L_STUB_FUNC(expbuf)
> +DEFINE_V4L_STUB_FUNC(g_std)
> +DEFINE_V4L_STUB_FUNC(g_audio)
> +DEFINE_V4L_STUB_FUNC(s_audio)
> +DEFINE_V4L_STUB_FUNC(g_input)
> +DEFINE_V4L_STUB_FUNC(g_edid)
> +DEFINE_V4L_STUB_FUNC(s_edid)
> +DEFINE_V4L_STUB_FUNC(g_output)
> +DEFINE_V4L_STUB_FUNC(g_audout)
>

Re: [PATCH v3] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

2018-05-08 Thread Hans Verkuil 
On 05/08/2018 07:35 PM, Sami Tolvanen wrote:
> This change fixes indirect call mismatches with Control-Flow Integrity
> checking, which are caused by calling standard ioctls using a function
> pointer that doesn't match the type of the actual function.

I really like the patch, but we've drifted away quite a bit from the original
patch, and I think the commit message needs to be updated a bit.

Just mention that IOCTL_INFO_STD has been removed, we now always call a function
and that DEFINE_V4L_STUB_FUNC is used to create stub functions where needed.

Basically the current message describes the problem (good), but it doesn't
describe the chosen solution (bad).

Sorry for asking for a v4, but this is core v4l2 code, so we're more careful
about this.

Regards,

Hans

> 
> Signed-off-by: Sami Tolvanen 
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
>  1 file changed, 130 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index de5d96dbe69e0..a40dbec271f1d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
>   unsigned int ioctl;
>   u32 flags;
>   const char * const name;
> - union {
> - u32 offset;
> - int (*func)(const struct v4l2_ioctl_ops *ops,
> - struct file *file, void *fh, void *p);
> - } u;
> + int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
> + void *fh, void *p);
>   void (*debug)(const void *arg, bool write_only);
>  };
>  
> @@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
>  #define INFO_FL_PRIO (1 << 0)
>  /* This control can be valid if the filehandle passes a control handler. */
>  #define INFO_FL_CTRL (1 << 1)
> -/* This is a standard ioctl, no need for special code */
> -#define INFO_FL_STD  (1 << 2)
> -/* This is ioctl has its own function */
> -#define INFO_FL_FUNC (1 << 3)
>  /* Queuing ioctl */
> -#define INFO_FL_QUEUE(1 << 4)
> +#define INFO_FL_QUEUE(1 << 2)
>  /* Always copy back result, even on error */
> -#define INFO_FL_ALWAYS_COPY  (1 << 5)
> +#define INFO_FL_ALWAYS_COPY  (1 << 3)
>  /* Zero struct from after the field to the end */
>  #define INFO_FL_CLEAR(v4l2_struct, field)\
>   ((offsetof(struct v4l2_struct, field) + \
> sizeof(((struct v4l2_struct *)0)->field)) << 16)
>  #define INFO_FL_CLEAR_MASK   (_IOC_SIZEMASK << 16)
>  
> -#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)  
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_STD,  \
> - .name = #_ioctl,\
> - .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
> - .debug = _debug,\
> +#define DEFINE_V4L_STUB_FUNC(_vidioc)\
> + static int v4l_stub_ ## _vidioc(\
> + const struct v4l2_ioctl_ops *ops,   \
> + struct file *file, void *fh, void *p)   \
> + {   \
> + return ops->vidioc_ ## _vidioc(file, fh, p);\
>   }
>  
> -#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_FUNC, \
> - .name = #_ioctl,\
> - .u.func = _func,\
> - .debug = _debug,\
> +#define IOCTL_INFO(_ioctl, _func, _debug, _flags)\
> + [_IOC_NR(_ioctl)] = {   \
> + .ioctl = _ioctl,\
> + .flags = _flags,\
> + .name = #_ioctl,\
> + .func = _func,  \
> + .debug = _debug,\
>   }
>  
> +DEFINE_V4L_STUB_FUNC(g_fbuf)
> +DEFINE_V4L_STUB_FUNC(s_fbuf)
> +DEFINE_V4L_STUB_FUNC(expbuf)
> +DEFINE_V4L_STUB_FUNC(g_std)
> +DEFINE_V4L_STUB_FUNC(g_audio)
> +DEFINE_V4L_STUB_FUNC(s_audio)
> +DEFINE_V4L_STUB_FUNC(g_input)
> +DEFINE_V4L_STUB_FUNC(g_edid)
> +DEFINE_V4L_STUB_FUNC(s_edid)
> +DEFINE_V4L_STUB_FUNC(g_output)
> +DEFINE_V4L_STUB_FUNC(g_audout)
> +DEFINE_V4L_STUB_FUNC(s_audout)
>

[PATCH v3] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

2018-05-08 Thread Sami Tolvanen 
This change fixes indirect call mismatches with Control-Flow Integrity
checking, which are caused by calling standard ioctls using a function
pointer that doesn't match the type of the actual function.

Signed-off-by: Sami Tolvanen 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
 1 file changed, 130 insertions(+), 115 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index de5d96dbe69e0..a40dbec271f1d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
unsigned int ioctl;
u32 flags;
const char * const name;
-   union {
-   u32 offset;
-   int (*func)(const struct v4l2_ioctl_ops *ops,
-   struct file *file, void *fh, void *p);
-   } u;
+   int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
+   void *fh, void *p);
void (*debug)(const void *arg, bool write_only);
 };
 
@@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
 #define INFO_FL_PRIO   (1 << 0)
 /* This control can be valid if the filehandle passes a control handler. */
 #define INFO_FL_CTRL   (1 << 1)
-/* This is a standard ioctl, no need for special code */
-#define INFO_FL_STD(1 << 2)
-/* This is ioctl has its own function */
-#define INFO_FL_FUNC   (1 << 3)
 /* Queuing ioctl */
-#define INFO_FL_QUEUE  (1 << 4)
+#define INFO_FL_QUEUE  (1 << 2)
 /* Always copy back result, even on error */
-#define INFO_FL_ALWAYS_COPY(1 << 5)
+#define INFO_FL_ALWAYS_COPY(1 << 3)
 /* Zero struct from after the field to the end */
 #define INFO_FL_CLEAR(v4l2_struct, field)  \
((offsetof(struct v4l2_struct, field) + \
  sizeof(((struct v4l2_struct *)0)->field)) << 16)
 #define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)
 
-#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)
\
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_STD,  \
-   .name = #_ioctl,\
-   .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
-   .debug = _debug,\
+#define DEFINE_V4L_STUB_FUNC(_vidioc)  \
+   static int v4l_stub_ ## _vidioc(\
+   const struct v4l2_ioctl_ops *ops,   \
+   struct file *file, void *fh, void *p)   \
+   {   \
+   return ops->vidioc_ ## _vidioc(file, fh, p);\
}
 
-#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)  \
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_FUNC, \
-   .name = #_ioctl,\
-   .u.func = _func,\
-   .debug = _debug,\
+#define IOCTL_INFO(_ioctl, _func, _debug, _flags)  \
+   [_IOC_NR(_ioctl)] = {   \
+   .ioctl = _ioctl,\
+   .flags = _flags,\
+   .name = #_ioctl,\
+   .func = _func,  \
+   .debug = _debug,\
}
 
+DEFINE_V4L_STUB_FUNC(g_fbuf)
+DEFINE_V4L_STUB_FUNC(s_fbuf)
+DEFINE_V4L_STUB_FUNC(expbuf)
+DEFINE_V4L_STUB_FUNC(g_std)
+DEFINE_V4L_STUB_FUNC(g_audio)
+DEFINE_V4L_STUB_FUNC(s_audio)
+DEFINE_V4L_STUB_FUNC(g_input)
+DEFINE_V4L_STUB_FUNC(g_edid)
+DEFINE_V4L_STUB_FUNC(s_edid)
+DEFINE_V4L_STUB_FUNC(g_output)
+DEFINE_V4L_STUB_FUNC(g_audout)
+DEFINE_V4L_STUB_FUNC(s_audout)
+DEFINE_V4L_STUB_FUNC(g_jpegcomp)
+DEFINE_V4L_STUB_FUNC(s_jpegcomp)
+DEFINE_V4L_STUB_FUNC(enumaudio)
+DEFINE_V4L_STUB_FUNC(enumaudout)
+DEFINE_V4L_STUB_FUNC(enum_framesizes)
+DEFINE_V4L_STUB_FUNC(enum_frameintervals)
+DEFINE_V4L_STUB_FUNC(g_enc_index)
+DEFINE_V4L_STUB_FUNC(encoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_encoder_cmd)
+DEFINE_V4L_STUB_FUNC(decoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_decoder_cmd)
+DEFINE_V4L_STUB_FUNC(s_dv_timings)
+DEFINE_V4L_STUB_FUNC(g_dv_timings)
+DEFINE_V4L_STUB_FUNC(enum_dv_timings)
+DEFINE_V4L_STUB_FUNC(query_dv_timings)
+DEFINE_V4L_STUB_FUNC(dv_timings_cap)
+
 static struct v4l2_ioctl_info v4l2_ioctls[] = {
-   IOCTL_INFO_FNC(VIDIOC_QUERYCAP,

[PATCH v3] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

2018-05-08 Thread Sami Tolvanen 
This change fixes indirect call mismatches with Control-Flow Integrity
checking, which are caused by calling standard ioctls using a function
pointer that doesn't match the type of the actual function.

Signed-off-by: Sami Tolvanen 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
 1 file changed, 130 insertions(+), 115 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index de5d96dbe69e0..a40dbec271f1d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
unsigned int ioctl;
u32 flags;
const char * const name;
-   union {
-   u32 offset;
-   int (*func)(const struct v4l2_ioctl_ops *ops,
-   struct file *file, void *fh, void *p);
-   } u;
+   int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
+   void *fh, void *p);
void (*debug)(const void *arg, bool write_only);
 };
 
@@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
 #define INFO_FL_PRIO   (1 << 0)
 /* This control can be valid if the filehandle passes a control handler. */
 #define INFO_FL_CTRL   (1 << 1)
-/* This is a standard ioctl, no need for special code */
-#define INFO_FL_STD(1 << 2)
-/* This is ioctl has its own function */
-#define INFO_FL_FUNC   (1 << 3)
 /* Queuing ioctl */
-#define INFO_FL_QUEUE  (1 << 4)
+#define INFO_FL_QUEUE  (1 << 2)
 /* Always copy back result, even on error */
-#define INFO_FL_ALWAYS_COPY(1 << 5)
+#define INFO_FL_ALWAYS_COPY(1 << 3)
 /* Zero struct from after the field to the end */
 #define INFO_FL_CLEAR(v4l2_struct, field)  \
((offsetof(struct v4l2_struct, field) + \
  sizeof(((struct v4l2_struct *)0)->field)) << 16)
 #define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)
 
-#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)
\
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_STD,  \
-   .name = #_ioctl,\
-   .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
-   .debug = _debug,\
+#define DEFINE_V4L_STUB_FUNC(_vidioc)  \
+   static int v4l_stub_ ## _vidioc(\
+   const struct v4l2_ioctl_ops *ops,   \
+   struct file *file, void *fh, void *p)   \
+   {   \
+   return ops->vidioc_ ## _vidioc(file, fh, p);\
}
 
-#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)  \
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_FUNC, \
-   .name = #_ioctl,\
-   .u.func = _func,\
-   .debug = _debug,\
+#define IOCTL_INFO(_ioctl, _func, _debug, _flags)  \
+   [_IOC_NR(_ioctl)] = {   \
+   .ioctl = _ioctl,\
+   .flags = _flags,\
+   .name = #_ioctl,\
+   .func = _func,  \
+   .debug = _debug,\
}
 
+DEFINE_V4L_STUB_FUNC(g_fbuf)
+DEFINE_V4L_STUB_FUNC(s_fbuf)
+DEFINE_V4L_STUB_FUNC(expbuf)
+DEFINE_V4L_STUB_FUNC(g_std)
+DEFINE_V4L_STUB_FUNC(g_audio)
+DEFINE_V4L_STUB_FUNC(s_audio)
+DEFINE_V4L_STUB_FUNC(g_input)
+DEFINE_V4L_STUB_FUNC(g_edid)
+DEFINE_V4L_STUB_FUNC(s_edid)
+DEFINE_V4L_STUB_FUNC(g_output)
+DEFINE_V4L_STUB_FUNC(g_audout)
+DEFINE_V4L_STUB_FUNC(s_audout)
+DEFINE_V4L_STUB_FUNC(g_jpegcomp)
+DEFINE_V4L_STUB_FUNC(s_jpegcomp)
+DEFINE_V4L_STUB_FUNC(enumaudio)
+DEFINE_V4L_STUB_FUNC(enumaudout)
+DEFINE_V4L_STUB_FUNC(enum_framesizes)
+DEFINE_V4L_STUB_FUNC(enum_frameintervals)
+DEFINE_V4L_STUB_FUNC(g_enc_index)
+DEFINE_V4L_STUB_FUNC(encoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_encoder_cmd)
+DEFINE_V4L_STUB_FUNC(decoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_decoder_cmd)
+DEFINE_V4L_STUB_FUNC(s_dv_timings)
+DEFINE_V4L_STUB_FUNC(g_dv_timings)
+DEFINE_V4L_STUB_FUNC(enum_dv_timings)
+DEFINE_V4L_STUB_FUNC(query_dv_timings)
+DEFINE_V4L_STUB_FUNC(dv_timings_cap)
+
 static struct v4l2_ioctl_info v4l2_ioctls[] = {
-   IOCTL_INFO_FNC(VIDIOC_QUERYCAP, v4l_querycap,