| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 03 Mar 2019 20:34:09 -0300
From: Ezequiel Garcia <ezequiel@...labora.com>
To: Shaobo He <shaobo@...utah.edu>, linux-media@...r.kernel.org
Cc: Philipp Zabel <p.zabel@...gutronix.de>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rick Chang <rick.chang@...iatek.com>,
Bin Liu <bin.liu@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Tiffany Lin <tiffany.lin@...iatek.com>,
Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
Mikhail Ulyanov <mikhail.ulyanov@...entembedded.com>,
Jacob chen <jacob2.chen@...k-chips.com>,
Heiko Stuebner <heiko@...ech.de>,
Kyungmin Park <kyungmin.park@...sung.com>,
Kamil Debski <kamil@...as.org>,
Andrzej Hajda <a.hajda@...sung.com>,
Andrzej Pietrasiewicz <andrzej.p@...sung.com>,
Jacek Anaszewski <jacek.anaszewski@...il.com>,
Benoit Parrot <bparrot@...com>,
Hans Verkuil <hans.verkuil@...co.com>,
Kees Cook <keescook@...omium.org>,
Anton Leontiev <scileont@...il.com>,
Simon Horman <horms+renesas@...ge.net.au>,
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
Tomasz Figa <tfiga@...omium.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
open list <linux-kernel@...r.kernel.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
"open list:MEDIA DRIVERS FOR RENESAS - FDP1"
<linux-renesas-soc@...r.kernel.org>,
"open list:ARM/Rockchip SoC support"
<linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH] Remove deductively redundant NULL pointer checks
On Tue, 2019-02-26 at 17:43 -0700, Shaobo He wrote:
> The fixes included in this commit essentially removes NULL pointer
> checks on the return values of function `get_queue_ctx` as well as
> `v4l2_m2m_get_vq` defined in file v4l2-mem2mem.c.
>
> Function `get_queue_ctx` is very unlikely to return a NULL pointer
> because its return value is an address composed of the base address
> pointed by `m2m_ctx` and an offset of field `out_q_ctx` or `cap_q_ctx`.
> Since the offset of either field is not 0, for the return value to be
> NULL, pointer `m2m_ctx` must be a very large unsigned value such that
> its addition to the offset overflows to NULL which may be undefined
> according to this post:
> https://wdtz.org/catching-pointer-overflow-bugs.html. Moreover, even if
> `m2m_ctx` is NULL, the return value cannot be NULL, either. Therefore, I
> think it is reasonable to conclude that the return value of function
> `get_queue_ctx` cannot be NULL.
>
> Given the return values of `get_queue_ctx` not being NULL, we can follow
> a similar reasoning to conclude that the return value of
> `v4l2_mem_get_vq` cannot be NULL since its return value is the same
> address as the return value of `get_queue_ctx`. Therefore, this patch
> also removes NULL pointer checks on the return values of
> `v4l2_mem_get_vq`.
>
> Signed-off-by: Shaobo He <shaobo@...utah.edu>
Hi Shaobo,
It seems this is v2 of 1551128631-19713-1-git-send-email-shaobo@...utah.edu,
and it should be marked as such.
Do you think you can read Documentation/process/submitting-patches.rst,
for your future patches?
Also, two comments...
> drivers/media/platform/coda/coda-common.c | 4 ----
> drivers/media/platform/imx-pxp.c | 7 -------
> drivers/media/platform/m2m-deinterlace.c | 7 -------
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 7 -------
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 7 -------
> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 13 -------------
> drivers/media/platform/mx2_emmaprp.c | 7 -------
> drivers/media/platform/rcar_fdp1.c | 3 ---
> drivers/media/platform/rcar_jpu.c | 8 --------
> drivers/media/platform/rockchip/rga/rga.c | 4 ----
> drivers/media/platform/s5p-g2d/g2d.c | 4 ----
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 7 -------
> drivers/media/platform/sh_veu.c | 2 --
> drivers/media/platform/ti-vpe/vpe.c | 7 -------
> drivers/media/platform/vicodec/vicodec-core.c | 5 -----
> drivers/media/platform/vim2m.c | 7 -------
> drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ----
> 17 files changed, 103 deletions(-)
>
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 7518f01..ee1e05b 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -696,8 +696,6 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct v4l2_format *f,
> struct vb2_queue *vq;
>
> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (!vq)
> - return -EINVAL;
>
> q_data = get_q_data(ctx, f->type);
> if (!q_data)
> @@ -817,8 +815,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
> ctx->quantization = f->fmt.pix.quantization;
>
> dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> - if (!dst_vq)
> - return -EINVAL;
>
> /*
> * Setting the capture queue format is not possible while the capture
> diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
> index c1c2554..d079b3c 100644
> --- a/drivers/media/platform/imx-pxp.c
> +++ b/drivers/media/platform/imx-pxp.c
> @@ -1071,13 +1071,8 @@ static int pxp_enum_fmt_vid_out(struct file *file, void *priv,
>
> static int pxp_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
> {
> - struct vb2_queue *vq;
> struct pxp_q_data *q_data;
>
> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (!vq)
> - return -EINVAL;
> -
It seems your patch also removes unused code, but this is not really explained in the
commit log.
Perhaps it is better to split all these changes on their own patch:
one patch to remove dead code, and then another patch to remove unneeded null checks.
And also, I think you should add some comments, either in v4l2_m2m_get_vq's declaration
or definition, explaining that the return value cannot be NULL.
I have to say: I'm not a fan of "improvement" patches in code paths that
are anything but hot... but knock yourself out!
Thanks,
Eze
Powered by blists - more mailing lists