Re: Dead code in v4l2-mem2mem.c?

Re: Dead code in v4l2-mem2mem.c?

[Laurent Pinchart]

Hi Shaobo,

First of all, could you please make sure you send future mails to the linux-
media mailing list in plain text only (no HTML) ? The mailing list server 
rejects HTML e-mails.

On Thursday 16 Feb 2017 16:08:25 Shaobo wrote:
> Hi there,
> 
> My name is Shaobo He and I am a graduate student at University of Utah. I am
> applying a static analysis tool to the Linux device drivers, looking for
> NULL pointer dereference and accidentally found a plausible dead code
> location in v4l2-mem2mem.c due to undefined behavior.
> 
> The following is the problematic code segment,
> 
> static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx
> *m2m_ctx,
> 						  enum v4l2_buf_type type)
> {
> 	if (V4L2_TYPE_IS_OUTPUT(type))
> 		return &m2m_ctx->out_q_ctx;
> 	else
> 		return &m2m_ctx->cap_q_ctx;
> }
> 
> struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
> 				    enum v4l2_buf_type type)
> {
> 	struct v4l2_m2m_queue_ctx *q_ctx;
> 
> 	q_ctx = get_queue_ctx(m2m_ctx, type);
> 	if (!q_ctx)
> 		return NULL;
> 
> 	return &q_ctx->q;
> }
> 
> `get_queue_ctx` returns a pointer value that is an addition of the base
> pointer address (`m2m_ctx`) to a non-zero offset. The following is the
> definition of struct v4l2_m2m_ctx,
> 
> struct v4l2_m2m_ctx {
> 	/* optional cap/out vb2 queues lock */
> 	struct mutex			*q_lock;
> 
> 	/* internal use only */
> 	struct v4l2_m2m_dev		*m2m_dev;
> 
> 	struct v4l2_m2m_queue_ctx	cap_q_ctx;
> 
> 	struct v4l2_m2m_queue_ctx	out_q_ctx;
> 
> 	/* For device job queue */
> 	struct list_head		queue;
> 	unsigned long			job_flags;
> 	wait_queue_head_t		finished;
> 
> 	void				*priv;
> };
> 
> There is a NULL test in a caller of `get_queue_ctx` (line 85), which appears
> problematic to me. I'm not sure if it is defined or feasible under the
> context of Linux kernel. This blog
> (https://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html)
> suggests that the NULL check can be optimized away because the only case
> that the return value can be NULL triggers pointer overflow, which is
> undefined.
> 
> Please let me know if it makes sense or not. Thanks for your time and I am
> looking forward to your reply.

The NULL check is indeed wrong. I believe that the m2m_ctx argument passed to 
the v4l2_m2m_get_vq() function should never be NULL. We will however need to 
audit drivers to make sure that's the case. The NULL check could then be 
removed. Alternatively we could check m2m_ctx above the get_queue_ctx() call, 
which wouldn't require auditing drivers. It's a safe option, but would likely 
result in an unneeded NULL check.

-- 
Regards,

Laurent Pinchart

RE: Dead code in v4l2-mem2mem.c?

[Shaobo]

Hi Laurent,

Thanks a lot for your reply.

I would like to also point out the inconsistency of using `v4l2_m2m_get_vq`
inside drivers/media/v4l2-core/v4l2-mem2mem.c and inside other files. It
appears to me almost all call sites of `v4l2_m2m_get_vq` in
drivers/media/v4l2-core/v4l2-mem2mem.c does not have NULL check afterwards
while in other files (e.g., drivers/media/platform/mx2_emmaprp.c) they do. I
was wondering if there is special assumption on this function in mem2mem.c.

Best,
Shaobo
-----Original Message-----
From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] 
Sent: 2017年2月17日 3:26
To: Shaobo <shaobo@cs.utah.edu>
Cc: linux-media@vger.kernel.org; mchehab@kernel.org; hverkuil@xs4all.nl;
sakari.ailus@linux.intel.com; ricardo.ribalda@gmail.com
Subject: Re: Dead code in v4l2-mem2mem.c?

Hi Shaobo,

First of all, could you please make sure you send future mails to the linux-
media mailing list in plain text only (no HTML) ? The mailing list server
rejects HTML e-mails.

On Thursday 16 Feb 2017 16:08:25 Shaobo wrote:
> Hi there,
> 
> My name is Shaobo He and I am a graduate student at University of 
> Utah. I am applying a static analysis tool to the Linux device 
> drivers, looking for NULL pointer dereference and accidentally found a 
> plausible dead code location in v4l2-mem2mem.c due to undefined behavior.
> 
> The following is the problematic code segment,
> 
> static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx 
> *m2m_ctx,
> 						  enum v4l2_buf_type type)
> {
> 	if (V4L2_TYPE_IS_OUTPUT(type))
> 		return &m2m_ctx->out_q_ctx;
> 	else
> 		return &m2m_ctx->cap_q_ctx;
> }
> 
> struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
> 				    enum v4l2_buf_type type)
> {
> 	struct v4l2_m2m_queue_ctx *q_ctx;
> 
> 	q_ctx = get_queue_ctx(m2m_ctx, type);
> 	if (!q_ctx)
> 		return NULL;
> 
> 	return &q_ctx->q;
> }
> 
> `get_queue_ctx` returns a pointer value that is an addition of the 
> base pointer address (`m2m_ctx`) to a non-zero offset. The following 
> is the definition of struct v4l2_m2m_ctx,
> 
> struct v4l2_m2m_ctx {
> 	/* optional cap/out vb2 queues lock */
> 	struct mutex			*q_lock;
> 
> 	/* internal use only */
> 	struct v4l2_m2m_dev		*m2m_dev;
> 
> 	struct v4l2_m2m_queue_ctx	cap_q_ctx;
> 
> 	struct v4l2_m2m_queue_ctx	out_q_ctx;
> 
> 	/* For device job queue */
> 	struct list_head		queue;
> 	unsigned long			job_flags;
> 	wait_queue_head_t		finished;
> 
> 	void				*priv;
> };
> 
> There is a NULL test in a caller of `get_queue_ctx` (line 85), which 
> appears problematic to me. I'm not sure if it is defined or feasible 
> under the context of Linux kernel. This blog
> (https://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html)
> suggests that the NULL check can be optimized away because the only 
> case that the return value can be NULL triggers pointer overflow, 
> which is undefined.
> 
> Please let me know if it makes sense or not. Thanks for your time and 
> I am looking forward to your reply.

The NULL check is indeed wrong. I believe that the m2m_ctx argument passed
to the v4l2_m2m_get_vq() function should never be NULL. We will however need
to audit drivers to make sure that's the case. The NULL check could then be
removed. Alternatively we could check m2m_ctx above the get_queue_ctx()
call, which wouldn't require auditing drivers. It's a safe option, but would
likely result in an unneeded NULL check.

--
Regards,

Laurent Pinchart

Re: Dead code in v4l2-mem2mem.c?

[Laurent Pinchart]

Hi Shaobo,

On Friday 17 Feb 2017 11:42:25 Shaobo wrote:
> Hi Laurent,
> 
> Thanks a lot for your reply.
> 
> I would like to also point out the inconsistency of using `v4l2_m2m_get_vq`
> inside drivers/media/v4l2-core/v4l2-mem2mem.c and inside other files. It
> appears to me almost all call sites of `v4l2_m2m_get_vq` in
> drivers/media/v4l2-core/v4l2-mem2mem.c does not have NULL check afterwards
> while in other files (e.g., drivers/media/platform/mx2_emmaprp.c) they do. I
> was wondering if there is special assumption on this function in mem2mem.c.

I don't see any case where the function could reasonably be called with a NULL 
context other than a severe driver bug. This being said, we need to audit the 
callers to make sure that's really the case. Would you like to do so and 
submit a patch ? :-)

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: 2017年2月17日 3:26
> To: Shaobo <shaobo@cs.utah.edu>
> Cc: linux-media@vger.kernel.org; mchehab@kernel.org; hverkuil@xs4all.nl;
> sakari.ailus@linux.intel.com; ricardo.ribalda@gmail.com
> Subject: Re: Dead code in v4l2-mem2mem.c?
> 
> Hi Shaobo,
> 
> First of all, could you please make sure you send future mails to the linux-
> media mailing list in plain text only (no HTML) ? The mailing list server
> rejects HTML e-mails.
> 
> On Thursday 16 Feb 2017 16:08:25 Shaobo wrote:
> > Hi there,
> > 
> > My name is Shaobo He and I am a graduate student at University of
> > Utah. I am applying a static analysis tool to the Linux device
> > drivers, looking for NULL pointer dereference and accidentally found a
> > plausible dead code location in v4l2-mem2mem.c due to undefined behavior.
> > 
> > The following is the problematic code segment,
> > 
> > static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx
> > *m2m_ctx,
> > 
> > 						  enum v4l2_buf_type type)
> > 
> > {
> > 
> > 	if (V4L2_TYPE_IS_OUTPUT(type))
> > 	
> > 		return &m2m_ctx->out_q_ctx;
> > 	
> > 	else
> > 	
> > 		return &m2m_ctx->cap_q_ctx;
> > 
> > }
> > 
> > struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
> > 
> > 				    enum v4l2_buf_type type)
> > 
> > {
> > 
> > 	struct v4l2_m2m_queue_ctx *q_ctx;
> > 	
> > 	q_ctx = get_queue_ctx(m2m_ctx, type);
> > 	if (!q_ctx)
> > 	
> > 		return NULL;
> > 	
> > 	return &q_ctx->q;
> > 
> > }
> > 
> > `get_queue_ctx` returns a pointer value that is an addition of the
> > base pointer address (`m2m_ctx`) to a non-zero offset. The following
> > is the definition of struct v4l2_m2m_ctx,
> > 
> > struct v4l2_m2m_ctx {
> > 
> > 	/* optional cap/out vb2 queues lock */
> > 	struct mutex			*q_lock;
> > 	
> > 	/* internal use only */
> > 	struct v4l2_m2m_dev		*m2m_dev;
> > 	
> > 	struct v4l2_m2m_queue_ctx	cap_q_ctx;
> > 	
> > 	struct v4l2_m2m_queue_ctx	out_q_ctx;
> > 	
> > 	/* For device job queue */
> > 	struct list_head		queue;
> > 	unsigned long			job_flags;
> > 	wait_queue_head_t		finished;
> > 	
> > 	void				*priv;
> > 
> > };
> > 
> > There is a NULL test in a caller of `get_queue_ctx` (line 85), which
> > appears problematic to me. I'm not sure if it is defined or feasible
> > under the context of Linux kernel. This blog
> > (https://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html)
> > suggests that the NULL check can be optimized away because the only
> > case that the return value can be NULL triggers pointer overflow,
> > which is undefined.
> > 
> > Please let me know if it makes sense or not. Thanks for your time and
> > I am looking forward to your reply.
> 
> The NULL check is indeed wrong. I believe that the m2m_ctx argument passed
> to the v4l2_m2m_get_vq() function should never be NULL. We will however need
> to audit drivers to make sure that's the case. The NULL check could then be
> removed. Alternatively we could check m2m_ctx above the get_queue_ctx()
> call, which wouldn't require auditing drivers. It's a safe option, but
> would likely result in an unneeded NULL check.
> 
> --
> Regards,
> 
> Laurent Pinchart

-- 
Regards,

Laurent Pinchart

RE: Dead code in v4l2-mem2mem.c?

[Shaobo]

Hi Laurent,

I'd like to. It sounds interesting and useful to me. Could you give me some pointers about how to audit drivers?

Shaobo
-----Original Message-----
From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] 
Sent: 2017年2月18日 3:54
To: Shaobo <shaobo@cs.utah.edu>
Cc: linux-media@vger.kernel.org; mchehab@kernel.org; hverkuil@xs4all.nl; sakari.ailus@linux.intel.com; ricardo.ribalda@gmail.com
Subject: Re: Dead code in v4l2-mem2mem.c?

Hi Shaobo,

On Friday 17 Feb 2017 11:42:25 Shaobo wrote:
> Hi Laurent,
> 
> Thanks a lot for your reply.
> 
> I would like to also point out the inconsistency of using 
> `v4l2_m2m_get_vq` inside drivers/media/v4l2-core/v4l2-mem2mem.c and 
> inside other files. It appears to me almost all call sites of 
> `v4l2_m2m_get_vq` in drivers/media/v4l2-core/v4l2-mem2mem.c does not 
> have NULL check afterwards while in other files (e.g., 
> drivers/media/platform/mx2_emmaprp.c) they do. I was wondering if there is special assumption on this function in mem2mem.c.

I don't see any case where the function could reasonably be called with a NULL context other than a severe driver bug. This being said, we need to audit the callers to make sure that's really the case. Would you like to do so and submit a patch ? :-)

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: 2017年2月17日 3:26
> To: Shaobo <shaobo@cs.utah.edu>
> Cc: linux-media@vger.kernel.org; mchehab@kernel.org; 
> hverkuil@xs4all.nl; sakari.ailus@linux.intel.com; 
> ricardo.ribalda@gmail.com
> Subject: Re: Dead code in v4l2-mem2mem.c?
> 
> Hi Shaobo,
> 
> First of all, could you please make sure you send future mails to the 
> linux- media mailing list in plain text only (no HTML) ? The mailing 
> list server rejects HTML e-mails.
> 
> On Thursday 16 Feb 2017 16:08:25 Shaobo wrote:
> > Hi there,
> > 
> > My name is Shaobo He and I am a graduate student at University of 
> > Utah. I am applying a static analysis tool to the Linux device 
> > drivers, looking for NULL pointer dereference and accidentally found 
> > a plausible dead code location in v4l2-mem2mem.c due to undefined behavior.
> > 
> > The following is the problematic code segment,
> > 
> > static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx 
> > *m2m_ctx,
> > 
> > 						  enum v4l2_buf_type type)
> > 
> > {
> > 
> > 	if (V4L2_TYPE_IS_OUTPUT(type))
> > 	
> > 		return &m2m_ctx->out_q_ctx;
> > 	
> > 	else
> > 	
> > 		return &m2m_ctx->cap_q_ctx;
> > 
> > }
> > 
> > struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
> > 
> > 				    enum v4l2_buf_type type)
> > 
> > {
> > 
> > 	struct v4l2_m2m_queue_ctx *q_ctx;
> > 	
> > 	q_ctx = get_queue_ctx(m2m_ctx, type);
> > 	if (!q_ctx)
> > 	
> > 		return NULL;
> > 	
> > 	return &q_ctx->q;
> > 
> > }
> > 
> > `get_queue_ctx` returns a pointer value that is an addition of the 
> > base pointer address (`m2m_ctx`) to a non-zero offset. The following 
> > is the definition of struct v4l2_m2m_ctx,
> > 
> > struct v4l2_m2m_ctx {
> > 
> > 	/* optional cap/out vb2 queues lock */
> > 	struct mutex			*q_lock;
> > 	
> > 	/* internal use only */
> > 	struct v4l2_m2m_dev		*m2m_dev;
> > 	
> > 	struct v4l2_m2m_queue_ctx	cap_q_ctx;
> > 	
> > 	struct v4l2_m2m_queue_ctx	out_q_ctx;
> > 	
> > 	/* For device job queue */
> > 	struct list_head		queue;
> > 	unsigned long			job_flags;
> > 	wait_queue_head_t		finished;
> > 	
> > 	void				*priv;
> > 
> > };
> > 
> > There is a NULL test in a caller of `get_queue_ctx` (line 85), which 
> > appears problematic to me. I'm not sure if it is defined or feasible 
> > under the context of Linux kernel. This blog
> > (https://wdtz.org/undefined-behavior-in-binutils-causes-segfault.htm
> > l) suggests that the NULL check can be optimized away because the 
> > only case that the return value can be NULL triggers pointer 
> > overflow, which is undefined.
> > 
> > Please let me know if it makes sense or not. Thanks for your time 
> > and I am looking forward to your reply.
> 
> The NULL check is indeed wrong. I believe that the m2m_ctx argument 
> passed to the v4l2_m2m_get_vq() function should never be NULL. We will 
> however need to audit drivers to make sure that's the case. The NULL 
> check could then be removed. Alternatively we could check m2m_ctx 
> above the get_queue_ctx() call, which wouldn't require auditing 
> drivers. It's a safe option, but would likely result in an unneeded NULL check.
> 
> --
> Regards,
> 
> Laurent Pinchart

--
Regards,

Laurent Pinchart

Re: Dead code in v4l2-mem2mem.c?

[Laurent Pinchart]

Hi Shaobo,

On Monday 20 Feb 2017 12:49:18 Shaobo wrote:
> Hi Laurent,
> 
> I'd like to. It sounds interesting and useful to me. Could you give me some
> pointers about how to audit drivers?

It's pretty simple, you need to check all functions that call get_queue_ctx() 
and follow the call stacks up to drivers to see if the context can be NULL. 
It's a bit of work though :-)

-- 
Regards,

Laurent Pinchart

Dead code in v4l2-mem2mem.c?

[Shaobo]

Hi there,

My name is Shaobo He and I am a graduate student at University of Utah. 
I am applying a static analysis tool to the Linux device drivers, 
looking for NULL pointer dereference and accidentally found a plausible 
dead code location in v4l2-mem2mem.c due to undefined behavior.

The following is the problematic code segment 
(drivers/media/v4l2-core/v4l2-mem2mem.c),

> 70 static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx 
> *m2m_ctx,
> 71                                                 enum v4l2_buf_type 
> type)
> 72 {
> 73         if (V4L2_TYPE_IS_OUTPUT(type))
> 74                 return &m2m_ctx->out_q_ctx;
> 75         else
> 76                 return &m2m_ctx->cap_q_ctx;
> 77 }
> 78
> 79 struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
> 80                                        enum v4l2_buf_type type)
> 81 {
> 82         struct v4l2_m2m_queue_ctx *q_ctx;
> 83
> 84         q_ctx = get_queue_ctx(m2m_ctx, type);
> 85         if (!q_ctx)
> 86                 return NULL;
> 87
> 88         return &q_ctx->q;
> 89 }

`get_queue_ctx` returns a pointer value that is an addition of the base 
pointer address (`m2m_ctx`) to a non-zero offset. The following is the 
definition of struct v4l2_m2m_ctx (include/media/v4l2-mem2mem.h),

> 94 struct v4l2_m2m_ctx {
> 95         /* optional cap/out vb2 queues lock */
> 96         struct mutex                    *q_lock;
> 97
> 98         /* internal use only */
> 99         struct v4l2_m2m_dev             *m2m_dev;
> 100
> 101         struct v4l2_m2m_queue_ctx       cap_q_ctx;
> 102
> 103         struct v4l2_m2m_queue_ctx       out_q_ctx;
> 104
> 105         /* For device job queue */
> 106         struct list_head                queue;
> 107         unsigned long                   job_flags;
> 108         wait_queue_head_t               finished;
> 109
> 110         void                            *priv;
> 111 };

There is a NULL test in a caller of `get_queue_ctx` (line 85), which 
appears problematic to me. I’m not sure if it is defined or feasible 
under the context of Linux kernel. This blog 
(https://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html) 
suggests that the NULL check can be optimized away because the only case 
that the return value can be NULL triggers pointer overflow, which is 
undefined.

Please let me know if it makes sense or not. Thanks for your time and I 
am looking forward to your reply.

Best,
Shaobo