| Message ID | 20180208144749.10558-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1ejnUo-0002ef-Bf; Thu, 08 Feb 2018 14:48:18 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752092AbeBHOsP (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 8 Feb 2018 09:48:15 -0500 Received: from metis.ext.pengutronix.de ([85.220.165.71]:45209 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750961AbeBHOsP (ORCPT <rfc822; linux-media@vger.kernel.org>); Thu, 8 Feb 2018 09:48:15 -0500 Received: from dude.hi.pengutronix.de ([2001:67c:670:100:1d::7] helo=dude.pengutronix.de.) by metis.ext.pengutronix.de with esmtp (Exim 4.89) (envelope-from <p.zabel@pengutronix.de>) id 1ejnUj-00051Z-R7; Thu, 08 Feb 2018 15:48:13 +0100 From: Philipp Zabel <p.zabel@pengutronix.de> To: linux-media@vger.kernel.org Cc: Steve Longerbeam <slongerbeam@gmail.com>, Hans Verkuil <hans.verkuil@cisco.com>, Philipp Zabel <p.zabel@pengutronix.de> Subject: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes Date: Thu, 8 Feb 2018 15:47:49 +0100 Message-Id: <20180208144749.10558-1-p.zabel@pengutronix.de> X-Mailer: git-send-email 2.15.1 X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::7 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-media@vger.kernel.org Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Philipp Zabel
Feb. 8, 2018, 2:47 p.m. UTC
If no imx_media_pixfmt is found for a given mbus format code,
we shouldn't crash. Return -EINVAL for any index.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/staging/media/imx/imx-media-csi.c | 4 ++++
1 file changed, 4 insertions(+)
Comments
On 02/08/2018 06:47 AM, Philipp Zabel wrote: > If no imx_media_pixfmt is found for a given mbus format code, > we shouldn't crash. Return -EINVAL for any index. Hi Philipp, If imx_media_find_mbus_format() returns NULL at that location, it means the current format has an invalid pixel code. It's not possible for an ACTIVE format to have an invalid code, so it must be a TRY format with an uninitialized (zero) code. Which makes sense if enum_mbus_code(TRY) pad op is called before set_fmt(TRY), at the sink pad, was *ever* called. Am I right? It looks there is another location where this could possibly happen, in csi_try_fmt(). In that case it would happen if set_fmt(TRY) is called on a source pad, before set_fmt(TRY) was ever called at the sink pad. That's a weird corner case because it's not clear what a set_fmt(TRY) at the source pads should choose for pixel code if there was never a set_fmt(TRY) at the sink pad. But perhaps the following should be added to this patch as well. It makes the assumption that the TRY code at the sink pad is the same as the default active code set from csi_registered(). Or maybe I should ask the question, what should drivers do in set_fmt(TRY) at their source pads, if there was no prior set_fmt(TRY) at their sink pads? Steve @@ -1281,6 +1281,11 @@ static void csi_try_fmt(struct csi_priv *priv, case CSI_SRC_PAD_IDMAC: incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true); + if (!incc) { + imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV, false); + incc = imx_media_find_mbus_format(code, + CS_SEL_YUV, false); + } sdformat->format.width = compose->width; sdformat->format.height = compose->height; > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/staging/media/imx/imx-media-csi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index eb7be5093a9d..89903f267d60 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -1138,6 +1138,10 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd, > > infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, code->which); > incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true); > + if (!incc) { > + ret = -EINVAL; > + goto out; > + } > > switch (code->pad) { > case CSI_SINK_PAD:
On 02/08/2018 08:27 PM, Steve Longerbeam wrote: > > > On 02/08/2018 06:47 AM, Philipp Zabel wrote: >> If no imx_media_pixfmt is found for a given mbus format code, >> we shouldn't crash. Return -EINVAL for any index. > > Hi Philipp, > > If imx_media_find_mbus_format() returns NULL at that location, it means > the current format has an invalid pixel code. It's not possible for an > ACTIVE > format to have an invalid code, so it must be a TRY format with an > uninitialized > (zero) code. Which makes sense if enum_mbus_code(TRY) pad op is called > before > set_fmt(TRY), at the sink pad, was *ever* called. Am I right? > > It looks there is another location where this could possibly happen, in > csi_try_fmt(). > In that case it would happen if set_fmt(TRY) is called on a source pad, > before > set_fmt(TRY) was ever called at the sink pad. That's a weird corner case > because > it's not clear what a set_fmt(TRY) at the source pads should choose for > pixel > code if there was never a set_fmt(TRY) at the sink pad. > > But perhaps the following should be added to this patch as well. It > makes the > assumption that the TRY code at the sink pad is the same as the default > active code set from csi_registered(). > > Or maybe I should ask the question, what should drivers do in set_fmt(TRY) > at their source pads, if there was no prior set_fmt(TRY) at their sink pads? Drivers can set the initial TRY value by implementing the init_cfg pad op. See e.g. drivers/media/platform/vimc/vimc-sensor.c. I think you have two choices: either set it to some default format, or copy the current ACTIVE format. It doesn't really matter, as long as there is something valid. Many drivers will likely fail this test. Regards, Hans > > Steve > > > @@ -1281,6 +1281,11 @@ static void csi_try_fmt(struct csi_priv *priv, > case CSI_SRC_PAD_IDMAC: > incc = imx_media_find_mbus_format(infmt->code, > CS_SEL_ANY, true); > + if (!incc) { > + imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV, > false); > + incc = imx_media_find_mbus_format(code, > + CS_SEL_YUV, false); > + } > > sdformat->format.width = compose->width; > sdformat->format.height = compose->height; > > > >> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >> --- >> drivers/staging/media/imx/imx-media-csi.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c >> index eb7be5093a9d..89903f267d60 100644 >> --- a/drivers/staging/media/imx/imx-media-csi.c >> +++ b/drivers/staging/media/imx/imx-media-csi.c >> @@ -1138,6 +1138,10 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd, >> >> infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, code->which); >> incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true); >> + if (!incc) { >> + ret = -EINVAL; >> + goto out; >> + } >> >> switch (code->pad) { >> case CSI_SINK_PAD: >
Hi Hans, On 02/08/2018 12:01 PM, Hans Verkuil wrote: > On 02/08/2018 08:27 PM, Steve Longerbeam wrote: >> >> On 02/08/2018 06:47 AM, Philipp Zabel wrote: >>> If no imx_media_pixfmt is found for a given mbus format code, >>> we shouldn't crash. Return -EINVAL for any index. >> Hi Philipp, >> >> If imx_media_find_mbus_format() returns NULL at that location, it means >> the current format has an invalid pixel code. It's not possible for an >> ACTIVE >> format to have an invalid code, so it must be a TRY format with an >> uninitialized >> (zero) code. Which makes sense if enum_mbus_code(TRY) pad op is called >> before >> set_fmt(TRY), at the sink pad, was *ever* called. Am I right? >> >> It looks there is another location where this could possibly happen, in >> csi_try_fmt(). >> In that case it would happen if set_fmt(TRY) is called on a source pad, >> before >> set_fmt(TRY) was ever called at the sink pad. That's a weird corner case >> because >> it's not clear what a set_fmt(TRY) at the source pads should choose for >> pixel >> code if there was never a set_fmt(TRY) at the sink pad. >> >> But perhaps the following should be added to this patch as well. It >> makes the >> assumption that the TRY code at the sink pad is the same as the default >> active code set from csi_registered(). >> >> Or maybe I should ask the question, what should drivers do in set_fmt(TRY) >> at their source pads, if there was no prior set_fmt(TRY) at their sink pads? > Drivers can set the initial TRY value by implementing the init_cfg pad op. > See e.g. drivers/media/platform/vimc/vimc-sensor.c. Thanks. I will send a patch that implements init_cfg pad op in all imx-media subdevs. Steve > > I think you have two choices: either set it to some default format, or copy > the current ACTIVE format. It doesn't really matter, as long as there is > something valid. > > Many drivers will likely fail this test. > > Regards, > > Hans > >
Hi Philipp, On 02/08/2018 12:01 PM, Hans Verkuil wrote: > On 02/08/2018 08:27 PM, Steve Longerbeam wrote: >> >> On 02/08/2018 06:47 AM, Philipp Zabel wrote: >>> If no imx_media_pixfmt is found for a given mbus format code, >>> we shouldn't crash. Return -EINVAL for any index. >> Hi Philipp, >> >> If imx_media_find_mbus_format() returns NULL at that location, it means >> the current format has an invalid pixel code. It's not possible for an >> ACTIVE >> format to have an invalid code, so it must be a TRY format with an >> uninitialized >> (zero) code. Which makes sense if enum_mbus_code(TRY) pad op is called >> before >> set_fmt(TRY), at the sink pad, was *ever* called. Am I right? >> >> It looks there is another location where this could possibly happen, in >> csi_try_fmt(). >> In that case it would happen if set_fmt(TRY) is called on a source pad, >> before >> set_fmt(TRY) was ever called at the sink pad. That's a weird corner case >> because >> it's not clear what a set_fmt(TRY) at the source pads should choose for >> pixel >> code if there was never a set_fmt(TRY) at the sink pad. >> >> But perhaps the following should be added to this patch as well. It >> makes the >> assumption that the TRY code at the sink pad is the same as the default >> active code set from csi_registered(). >> >> Or maybe I should ask the question, what should drivers do in set_fmt(TRY) >> at their source pads, if there was no prior set_fmt(TRY) at their sink pads? > Drivers can set the initial TRY value by implementing the init_cfg pad op. > See e.g. drivers/media/platform/vimc/vimc-sensor.c. I *think* by implementing init_cfg in the CSI, it will prevent the NULL deref in csi_enum_mbus_code(). However I think this patch is a good idea in any case. Steve > >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> drivers/staging/media/imx/imx-media-csi.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c >>> index eb7be5093a9d..89903f267d60 100644 >>> --- a/drivers/staging/media/imx/imx-media-csi.c >>> +++ b/drivers/staging/media/imx/imx-media-csi.c >>> @@ -1138,6 +1138,10 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd, >>> >>> infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, code->which); >>> incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true); >>> + if (!incc) { >>> + ret = -EINVAL; >>> + goto out; >>> + } >>> >>> switch (code->pad) { >>> case CSI_SINK_PAD:
Hi Steve, On Fri, 2018-02-09 at 17:43 -0800, Steve Longerbeam wrote: [...] > I *think* by implementing init_cfg in the CSI, it will prevent the > NULL deref in csi_enum_mbus_code(). However I think this patch > is a good idea in any case. Ack on both. Can we still get this patch applied? regards Philipp
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index eb7be5093a9d..89903f267d60 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -1138,6 +1138,10 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd, infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, code->which); incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true); + if (!incc) { + ret = -EINVAL; + goto out; + } switch (code->pad) { case CSI_SINK_PAD: