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