| Message ID | 5963491651fe2385fa50cf9371cb826f640e91e8.1523024380.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 06, 2018 at 10:23:04AM -0400, Mauro Carvalho Chehab wrote: > As warned: > drivers/staging/media/davinci_vpfe/dm365_ipipe.c:1834 vpfe_ipipe_init() error: we previously assumed 'res' could be null (see line 1797) > > There's something wrong at vpfe_ipipe_init(): > > 1) it caches the resourse_size() from from the first region > and reuses to the second region; > > 2) the "res" var is overriden 3 times; > > 3) at free logic, it assumes that "res->start" is not > overriden by platform_get_resource(pdev, IORESOURCE_MEM, 6), > but that's not true, as it can even be NULL there. > > This patch fixes the above issues by: > > a) store the resources used by release_mem_region() on > a separate var; > > b) stop caching resource_size(), using the function where > needed. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> I ran coccicheck on a 4.14.74 stable kernel and noticed that 'res' can be NULL in vpfe_ipipe_init. It looks like this patch is not included in the 4.14 stable series. Can this patch be applied? I applied it myself and it applies cleanly, but I have no way to test it. That 'res->start' error_release could end up a NULL pointer deref. - Joel
On Mon, Oct 08, 2018 at 09:46:01PM -0700, Joel Fernandes wrote: > On Fri, Apr 06, 2018 at 10:23:04AM -0400, Mauro Carvalho Chehab wrote: > > As warned: > > drivers/staging/media/davinci_vpfe/dm365_ipipe.c:1834 vpfe_ipipe_init() error: we previously assumed 'res' could be null (see line 1797) > > > > There's something wrong at vpfe_ipipe_init(): > > > > 1) it caches the resourse_size() from from the first region > > and reuses to the second region; > > > > 2) the "res" var is overriden 3 times; > > > > 3) at free logic, it assumes that "res->start" is not > > overriden by platform_get_resource(pdev, IORESOURCE_MEM, 6), > > but that's not true, as it can even be NULL there. > > > > This patch fixes the above issues by: > > > > a) store the resources used by release_mem_region() on > > a separate var; > > > > b) stop caching resource_size(), using the function where > > needed. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > I ran coccicheck on a 4.14.74 stable kernel and noticed that 'res' can be > NULL in vpfe_ipipe_init. It looks like this patch is not included in the 4.14 > stable series. Can this patch be applied? I applied it myself and it applies > cleanly, but I have no way to test it. > > That 'res->start' error_release could end up a NULL pointer deref. Should this patch goto 4.14 stable? Seems straightforward and worth it to prevent the possible NULL pointer deref issue. - Joel
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index dd0cfc79f50c..b3a193ddd778 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1778,25 +1778,25 @@ vpfe_ipipe_init(struct vpfe_ipipe_device *ipipe, struct platform_device *pdev) struct media_pad *pads = &ipipe->pads[0]; struct v4l2_subdev *sd = &ipipe->subdev; struct media_entity *me = &sd->entity; - static resource_size_t res_len; - struct resource *res; + struct resource *res, *memres; res = platform_get_resource(pdev, IORESOURCE_MEM, 4); if (!res) return -ENOENT; - res_len = resource_size(res); - res = request_mem_region(res->start, res_len, res->name); - if (!res) + memres = request_mem_region(res->start, resource_size(res), res->name); + if (!memres) return -EBUSY; - ipipe->base_addr = ioremap_nocache(res->start, res_len); + ipipe->base_addr = ioremap_nocache(memres->start, + resource_size(memres)); if (!ipipe->base_addr) goto error_release; res = platform_get_resource(pdev, IORESOURCE_MEM, 6); if (!res) goto error_unmap; - ipipe->isp5_base_addr = ioremap_nocache(res->start, res_len); + ipipe->isp5_base_addr = ioremap_nocache(res->start, + resource_size(res)); if (!ipipe->isp5_base_addr) goto error_unmap; @@ -1831,7 +1831,7 @@ vpfe_ipipe_init(struct vpfe_ipipe_device *ipipe, struct platform_device *pdev) error_unmap: iounmap(ipipe->base_addr); error_release: - release_mem_region(res->start, res_len); + release_mem_region(memres->start, resource_size(memres)); return -ENOMEM; }
As warned: drivers/staging/media/davinci_vpfe/dm365_ipipe.c:1834 vpfe_ipipe_init() error: we previously assumed 'res' could be null (see line 1797) There's something wrong at vpfe_ipipe_init(): 1) it caches the resourse_size() from from the first region and reuses to the second region; 2) the "res" var is overriden 3 times; 3) at free logic, it assumes that "res->start" is not overriden by platform_get_resource(pdev, IORESOURCE_MEM, 6), but that's not true, as it can even be NULL there. This patch fixes the above issues by: a) store the resources used by release_mem_region() on a separate var; b) stop caching resource_size(), using the function where needed. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> --- drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)