Re: Dead code in v4l2-mem2mem.c?
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
RE: Dead code in v4l2-mem2mem.c?
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.pinch...@ideasonboard.com]
Sent: 2017年2月18日 3:54
To: Shaobo <sha...@cs.utah.edu>
Cc: linux-media@vger.kernel.org; mche...@kernel.org; hverk...@xs4all.nl;
sakari.ai...@linux.intel.com; ricardo.riba...@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.pinch...@ideasonboard.com]
> Sent: 2017年2月17日 3:26
> To: Shaobo <sha...@cs.utah.edu>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org;
> hverk...@xs4all.nl; sakari.ai...@linux.intel.com;
> ricardo.riba...@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 _ctx->out_q_ctx;
> >
> > else
> >
> > return _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 _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_headqueue;
> > 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?
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.pinch...@ideasonboard.com]
> Sent: 2017年2月17日 3:26
> To: Shaobo <sha...@cs.utah.edu>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; hverk...@xs4all.nl;
> sakari.ai...@linux.intel.com; ricardo.riba...@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 _ctx->out_q_ctx;
> >
> > else
> >
> > return _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 _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_headqueue;
> > 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?
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.pinch...@ideasonboard.com]
Sent: 2017年2月17日 3:26
To: Shaobo <sha...@cs.utah.edu>
Cc: linux-media@vger.kernel.org; mche...@kernel.org; hverk...@xs4all.nl;
sakari.ai...@linux.intel.com; ricardo.riba...@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 _ctx->out_q_ctx;
> else
> return _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 _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_headqueue;
> 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?
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 _ctx->out_q_ctx;
> else
> return _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 _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_headqueue;
> 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
Dead code in v4l2-mem2mem.c?
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 _ctx->out_q_ctx;
75 else
76 return _ctx->cap_q_ctx;
77 }
78
79 struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
80enum 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 _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_headqueue;
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
