* [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
@ 2018-02-08 14:47 Philipp Zabel
2018-02-08 19:27 ` Steve Longerbeam
0 siblings, 1 reply; 6+ messages in thread
From: Philipp Zabel @ 2018-02-08 14:47 UTC (permalink / raw
To: linux-media; +Cc: Steve Longerbeam, Hans Verkuil, Philipp Zabel
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(+)
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:
--
2.15.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
2018-02-08 14:47 [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes Philipp Zabel
@ 2018-02-08 19:27 ` Steve Longerbeam
2018-02-08 20:01 ` Hans Verkuil
0 siblings, 1 reply; 6+ messages in thread
From: Steve Longerbeam @ 2018-02-08 19:27 UTC (permalink / raw
To: Philipp Zabel, linux-media; +Cc: Hans Verkuil
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:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
2018-02-08 19:27 ` Steve Longerbeam
@ 2018-02-08 20:01 ` Hans Verkuil
2018-02-10 1:22 ` Steve Longerbeam
2018-02-10 1:43 ` Steve Longerbeam
0 siblings, 2 replies; 6+ messages in thread
From: Hans Verkuil @ 2018-02-08 20:01 UTC (permalink / raw
To: Steve Longerbeam, Philipp Zabel, linux-media; +Cc: Hans Verkuil
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:
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
2018-02-08 20:01 ` Hans Verkuil
@ 2018-02-10 1:22 ` Steve Longerbeam
2018-02-10 1:43 ` Steve Longerbeam
1 sibling, 0 replies; 6+ messages in thread
From: Steve Longerbeam @ 2018-02-10 1:22 UTC (permalink / raw
To: Hans Verkuil, Philipp Zabel, linux-media; +Cc: Hans Verkuil
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
2018-02-08 20:01 ` Hans Verkuil
2018-02-10 1:22 ` Steve Longerbeam
@ 2018-02-10 1:43 ` Steve Longerbeam
2018-11-05 14:59 ` Philipp Zabel
1 sibling, 1 reply; 6+ messages in thread
From: Steve Longerbeam @ 2018-02-10 1:43 UTC (permalink / raw
To: Philipp Zabel; +Cc: Hans Verkuil, linux-media, Hans Verkuil
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:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
2018-02-10 1:43 ` Steve Longerbeam
@ 2018-11-05 14:59 ` Philipp Zabel
0 siblings, 0 replies; 6+ messages in thread
From: Philipp Zabel @ 2018-11-05 14:59 UTC (permalink / raw
To: Steve Longerbeam, Hans Verkuil; +Cc: linux-media, Hans Verkuil
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-06 0:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-08 14:47 [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes Philipp Zabel
2018-02-08 19:27 ` Steve Longerbeam
2018-02-08 20:01 ` Hans Verkuil
2018-02-10 1:22 ` Steve Longerbeam
2018-02-10 1:43 ` Steve Longerbeam
2018-11-05 14:59 ` Philipp Zabel
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.