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_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 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
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 &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_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,
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
> 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 &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_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,
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
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
> 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 &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_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 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
