* Problematic code in media/v4l2-core/v4l2-mem2mem.c @ 2019-02-21 18:11 Shaobo 2019-02-21 21:50 ` Laurent Pinchart 0 siblings, 1 reply; 3+ messages in thread From: Shaobo @ 2019-02-21 18:11 UTC (permalink / raw) To: linux-media, pawel, m.szyprowski, laurent.pinchart 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Problematic code in media/v4l2-core/v4l2-mem2mem.c 2019-02-21 18:11 Problematic code in media/v4l2-core/v4l2-mem2mem.c Shaobo @ 2019-02-21 21:50 ` Laurent Pinchart 2019-02-22 0:27 ` Shaobo He 0 siblings, 1 reply; 3+ messages in thread From: Laurent Pinchart @ 2019-02-21 21:50 UTC (permalink / raw) To: Shaobo; +Cc: linux-media, pawel, m.szyprowski 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Problematic code in media/v4l2-core/v4l2-mem2mem.c 2019-02-21 21:50 ` Laurent Pinchart @ 2019-02-22 0:27 ` Shaobo He 0 siblings, 0 replies; 3+ messages in thread From: Shaobo He @ 2019-02-22 0:27 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, pawel, m.szyprowski 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 ? :-) > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-22 0:27 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-21 18:11 Problematic code in media/v4l2-core/v4l2-mem2mem.c Shaobo 2019-02-21 21:50 ` Laurent Pinchart 2019-02-22 0:27 ` Shaobo He
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.