Problematic code in media/v4l2-core/v4l2-mem2mem.c

Problematic code in media/v4l2-core/v4l2-mem2mem.c

[Shaobo]

Hello everyone,

I think I brought up this issue before but didn't resolve it completely. 
Now I'd like to double check this and if we can agree on it, I'd also 
like to submit a patch to fix it. The problem is that function 
`get_queue_ctx` can never return a NULL pointer unless pointer overflow 
occurs, which is very unlikely. To be more specific,

```
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;
}
```

The address returned by this function is either `(char*)m2m_ctx+968` or 
`(char*)m2m_ctx+16`, so for it to be NULL, `m2m_ctx` must be a large 
unsigned value. Yet the return value of this function is NULL-checked, 
for example in v4l2_m2m_get_vq.

Please let me know if it makes sense.

Best,
Shaobo

Re: Problematic code in media/v4l2-core/v4l2-mem2mem.c

[Laurent Pinchart]

Hi Shaobo,

On Thu, Feb 21, 2019 at 11:11:32AM -0700, Shaobo wrote:
> Hello everyone,
> 
> I think I brought up this issue before but didn't resolve it completely. 
> Now I'd like to double check this and if we can agree on it, I'd also 
> like to submit a patch to fix it. The problem is that function 
> `get_queue_ctx` can never return a NULL pointer unless pointer overflow 
> occurs, which is very unlikely. To be more specific,
> 
> ```
> 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;
> }
> ```
> 
> The address returned by this function is either `(char*)m2m_ctx+968` or 
> `(char*)m2m_ctx+16`, so for it to be NULL, `m2m_ctx` must be a large 
> unsigned value. Yet the return value of this function is NULL-checked, 
> for example in v4l2_m2m_get_vq.
> 
> Please let me know if it makes sense.

It makes complete sense.

There are only two callers of get_queue_ctx() that check the return
value of the function. It may be argued that the intent was to check for
a NULL m2m_ctx, so you could replace those two checks with a NULL check
for m2m_ctx before calling get_queue_ctx(). However, given that nothing
is crashing, it may also be argued that the checks are unnecessary and
can be dropped completely. The best would be to review the call paths to
ensure the functions can indeed never be called with NULL, but a quick
look at the code shows no other NULL check in functions taking a m2m_ctx
pointer as argument, so I'd vote for just dropping the two offending
checks.

Care to submit a patch ? :-)

-- 
Regards,

Laurent Pinchart

Re: Problematic code in media/v4l2-core/v4l2-mem2mem.c

[Shaobo He]

Hi Laurent,

Thank you very much for your reply. While I was working on a patch, I realized 
that if we drop the two problematic NULL checks, we essentially eliminate one 
path of `v4l2_m2m_get_vq` where a NULL pointer is returned. So here comes a 
question, we may want to remove all the NULL checks of the return values of this 
function, for example in line 574 of media/platform/coda/coda-common.c. The 
reason is that if we conclude that `get_queue_ctx` cannot return a NULL pointer, 
then &q_ctx->q cannot be a NULL pointer as well because it points to the same 
address as `q_ctx` does.

If we agree on the NULL checks of the return values of `v4l2_m2m_get_vq` being 
unnecessary, I'll remove them in the patch.

Best,
Shaobo
On 2019/2/21 14:50, Laurent Pinchart wrote:
> Hi Shaobo,
> 
> On Thu, Feb 21, 2019 at 11:11:32AM -0700, Shaobo wrote:
>> Hello everyone,
>>
>> I think I brought up this issue before but didn't resolve it completely.
>> Now I'd like to double check this and if we can agree on it, I'd also
>> like to submit a patch to fix it. The problem is that function
>> `get_queue_ctx` can never return a NULL pointer unless pointer overflow
>> occurs, which is very unlikely. To be more specific,
>>
>> ```
>> 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;
>> }
>> ```
>>
>> The address returned by this function is either `(char*)m2m_ctx+968` or
>> `(char*)m2m_ctx+16`, so for it to be NULL, `m2m_ctx` must be a large
>> unsigned value. Yet the return value of this function is NULL-checked,
>> for example in v4l2_m2m_get_vq.
>>
>> Please let me know if it makes sense.
> 
> It makes complete sense.
> 
> There are only two callers of get_queue_ctx() that check the return
> value of the function. It may be argued that the intent was to check for
> a NULL m2m_ctx, so you could replace those two checks with a NULL check
> for m2m_ctx before calling get_queue_ctx(). However, given that nothing
> is crashing, it may also be argued that the checks are unnecessary and
> can be dropped completely. The best would be to review the call paths to
> ensure the functions can indeed never be called with NULL, but a quick
> look at the code shows no other NULL check in functions taking a m2m_ctx
> pointer as argument, so I'd vote for just dropping the two offending
> checks.
> 
> Care to submit a patch ? :-)
>