Potentially invalid memory accesses in file drivers/media/v4l2-core/videobuf-core.c
Hi there,
My name is Shaobo He and I am a graduate student at University of Utah.
I am using a static analysis tool to search for null pointer
dereferences and came across a couple of potentially invalid memory
accesses in the file drivers/media/v4l2-core/videobuf-core.c. Basically
the expansion of the macro `CALL_PTR` is never examined although it can
evaluate to NULL. The following is its definition and two uses:
#define CALLPTR(q, f, arg...) \
((q->int_ops->f) ? q->int_ops->f(arg) : NULL)
static int __videobuf_copy_to_user(struct videobuf_queue *q,
struct videobuf_buffer *buf,
char __user *data, size_t count,
int nonblocking)
{
void *vaddr = CALLPTR(q, vaddr, buf);
/* copy to userspace */
if (count > buf->size - q->read_off)
count = buf->size - q->read_off;
if (copy_to_user(data, vaddr + q->read_off, count))
return -EFAULT;
return count;
}
static int __videobuf_copy_stream(struct videobuf_queue *q,
struct videobuf_buffer *buf,
char __user *data, size_t count, size_t pos,
int vbihack, int nonblocking)
{
unsigned int *fc = CALLPTR(q, vaddr, buf);
if (vbihack) {
/* dirty, undocumented hack -- pass the frame counter
* within the last four bytes of each vbi data block.
* We need that one to maintain backward compatibility
* to all vbi decoding software out there ... */
fc += (buf->size >> 2) - 1;
*fc = buf->field_count >> 1;
dprintk(1, "vbihack: %d\n", *fc);
}
/* copy stuff using the common method */
count = __videobuf_copy_to_user(q, buf, data, count, nonblocking);
if ((count == -EFAULT) && (pos == 0))
return -EFAULT;
return count;
}
Both of the two functions could contain invalid memory accesses. The
second function `__videobuf_copy_stream` is more problematic since if
`buf-?size >> 2` evaluates to 1, which seems not totally impossible to
me, then a NULL pointer dereference would occur.
Please let me know if it makes sense. Thanks for your time and I am
looking forward to your reply.
Shaobo
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
Dead code or otherwise invalid memory access in drivers/media/v4l2-core/videobuf-core.c
Hey guys, I found that the definition and usage of macro `CALLPTR` may be problematic. Its definition is, 54 #define CALLPTR(q, f, arg...) \ 55 ((q->int_ops->f) ? q->int_ops->f(arg) : NULL) , which means it can evaluate to NULL. It has two occurrences: one in line 839 and the other is line 856. It appears to me that it's very likely that there will be invalid memory accesses if `CALLPTR` evaluates to NULL since there is no NULL test in either location. In other words, programmers' assumption suggest the else branch of the conditional expression dead. Please let me know if makes sense or not. Thanks for your time and I am looking forward to your reply. Best, Shaobo
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
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
