[PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

I fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
 drivers/staging/media/meson/vdec/vdec_helpers.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..41297c2f8f9a 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -234,6 +234,11 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts) {
+		dev_err(sess->core->dev_dec,
+			"No enough memory in %s\n", __func__);
+		return;
+	}
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
-- 
2.25.1

[PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

I fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
 drivers/staging/media/meson/vdec/vdec_helpers.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..41297c2f8f9a 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -234,6 +234,11 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts) {
+		dev_err(sess->core->dev_dec,
+			"No enough memory in %s\n", __func__);
+		return;
+	}
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
-- 
2.25.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

I fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
 drivers/staging/media/meson/vdec/vdec_helpers.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..41297c2f8f9a 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -234,6 +234,11 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts) {
+		dev_err(sess->core->dev_dec,
+			"No enough memory in %s\n", __func__);
+		return;
+	}
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Dan Carpenter]

On Wed, Dec 01, 2021 at 12:12:23AM +0800, Zhou Qingyang wrote:
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..41297c2f8f9a 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -234,6 +234,11 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts) {
> +		dev_err(sess->core->dev_dec,
> +			"No enough memory in %s\n", __func__);
> +		return;

Please run checkpatch.pl on your patches.  The dev_err() message should
be deleted because kzalloc() already has a better message built in.

WARNING: Possible unnecessary 'out of memory' message
#50: FILE: drivers/staging/media/meson/vdec/vdec_helpers.c:238:
+       if (!new_ts) {
+               dev_err(sess->core->dev_dec,

regards,
dan carpenter

Re: [PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Dan Carpenter]

On Wed, Dec 01, 2021 at 12:12:23AM +0800, Zhou Qingyang wrote:
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..41297c2f8f9a 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -234,6 +234,11 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts) {
> +		dev_err(sess->core->dev_dec,
> +			"No enough memory in %s\n", __func__);
> +		return;

Please run checkpatch.pl on your patches.  The dev_err() message should
be deleted because kzalloc() already has a better message built in.

WARNING: Possible unnecessary 'out of memory' message
#50: FILE: drivers/staging/media/meson/vdec/vdec_helpers.c:238:
+       if (!new_ts) {
+               dev_err(sess->core->dev_dec,

regards,
dan carpenter


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Dan Carpenter]

On Wed, Dec 01, 2021 at 12:12:23AM +0800, Zhou Qingyang wrote:
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..41297c2f8f9a 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -234,6 +234,11 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts) {
> +		dev_err(sess->core->dev_dec,
> +			"No enough memory in %s\n", __func__);
> +		return;

Please run checkpatch.pl on your patches.  The dev_err() message should
be deleted because kzalloc() already has a better message built in.

WARNING: Possible unnecessary 'out of memory' message
#50: FILE: drivers/staging/media/meson/vdec/vdec_helpers.c:238:
+       if (!new_ts) {
+               dev_err(sess->core->dev_dec,

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

I fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
Changes in v2:
  -  Delete dev_err() message

 drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..ac60514c475b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts)
+		return;
+
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
-- 
2.25.1

[PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

I fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
Changes in v2:
  -  Delete dev_err() message

 drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..ac60514c475b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts)
+		return;
+
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
-- 
2.25.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

I fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---
Changes in v2:
  -  Delete dev_err() message

 drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..ac60514c475b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts)
+		return;
+
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Dan Carpenter]

On Fri, Dec 03, 2021 at 12:03:57AM +0800, Zhou Qingyang wrote:
> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> I fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
  ^^^
Thanks.  Next time put the meta commentary about how the bug was found
and the QC process under the the --- cut off line.  We don't need to
have that drama stored in the permanent git log.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Dan Carpenter]

On Fri, Dec 03, 2021 at 12:03:57AM +0800, Zhou Qingyang wrote:
> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> I fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
  ^^^
Thanks.  Next time put the meta commentary about how the bug was found
and the QC process under the the --- cut off line.  We don't need to
have that drama stored in the permanent git log.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Dan Carpenter]

On Fri, Dec 03, 2021 at 12:03:57AM +0800, Zhou Qingyang wrote:
> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> I fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
  ^^^
Thanks.  Next time put the meta commentary about how the bug was found
and the QC process under the the --- cut off line.  We don't need to
have that drama stored in the permanent git log.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Mauro Carvalho Chehab]

Em Fri,  3 Dec 2021 00:03:57 +0800
Zhou Qingyang <zhou1615@umn.edu> escreveu:

> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> I fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> Changes in v2:
>   -  Delete dev_err() message
> 
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..ac60514c475b 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts)
> +		return;
> +
>  	new_ts->ts = ts;
>  	new_ts->tc = tc;
>  	new_ts->offset = offset;

I don't think this change is ok. Sure, it needs to check if
kzalloc() fails, but it should return -ENOMEM and the caller
should check if it returns an error. So, I would expect
that this patch would also touch the caller function at
drivers/staging/media/meson/vdec/esparser.c.

Regards,
Mauro



Thanks,
Mauro

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Mauro Carvalho Chehab]

Em Fri,  3 Dec 2021 00:03:57 +0800
Zhou Qingyang <zhou1615@umn.edu> escreveu:

> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> I fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> Changes in v2:
>   -  Delete dev_err() message
> 
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..ac60514c475b 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts)
> +		return;
> +
>  	new_ts->ts = ts;
>  	new_ts->tc = tc;
>  	new_ts->offset = offset;

I don't think this change is ok. Sure, it needs to check if
kzalloc() fails, but it should return -ENOMEM and the caller
should check if it returns an error. So, I would expect
that this patch would also touch the caller function at
drivers/staging/media/meson/vdec/esparser.c.

Regards,
Mauro



Thanks,
Mauro

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Mauro Carvalho Chehab]

Em Fri,  3 Dec 2021 00:03:57 +0800
Zhou Qingyang <zhou1615@umn.edu> escreveu:

> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> I fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> Changes in v2:
>   -  Delete dev_err() message
> 
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..ac60514c475b 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts)
> +		return;
> +
>  	new_ts->ts = ts;
>  	new_ts->tc = tc;
>  	new_ts->offset = offset;

I don't think this change is ok. Sure, it needs to check if
kzalloc() fails, but it should return -ENOMEM and the caller
should check if it returns an error. So, I would expect
that this patch would also touch the caller function at
drivers/staging/media/meson/vdec/esparser.c.

Regards,
Mauro



Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Greg Kroah-Hartman]

On Tue, Dec 14, 2021 at 02:46:13PM +0100, Mauro Carvalho Chehab wrote:
> Em Fri,  3 Dec 2021 00:03:57 +0800
> Zhou Qingyang <zhou1615@umn.edu> escreveu:
> 
> > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> > to a NULL pointer dereference on failure of kzalloc().
> > 
> > I fix this bug by adding a NULL check of new_ts.
> > 
> > This bug was found by a static analyzer. The analysis employs
> > differential checking to identify inconsistent security operations
> > (e.g., checks or kfrees) between two code paths and confirms that the
> > inconsistent operations are not recovered in the current function or
> > the callers, so they constitute bugs.
> > 
> > Note that, as a bug found by static analysis, it can be a false
> > positive or hard to trigger. Multiple researchers have cross-reviewed
> > the bug.
> > 
> > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> > and our static analyzer no longer warns about this code.
> > 
> > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> > ---
> > Changes in v2:
> >   -  Delete dev_err() message
> > 
> >  drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> > index b9125c295d1d..ac60514c475b 100644
> > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> > @@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> >  	unsigned long flags;
> >  
> >  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> > +	if (!new_ts)
> > +		return;
> > +
> >  	new_ts->ts = ts;
> >  	new_ts->tc = tc;
> >  	new_ts->offset = offset;
> 
> I don't think this change is ok. Sure, it needs to check if
> kzalloc() fails, but it should return -ENOMEM and the caller
> should check if it returns an error. So, I would expect
> that this patch would also touch the caller function at
> drivers/staging/media/meson/vdec/esparser.c.

This is why umn.edu emails still are in my black-hole :(

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Greg Kroah-Hartman]

On Tue, Dec 14, 2021 at 02:46:13PM +0100, Mauro Carvalho Chehab wrote:
> Em Fri,  3 Dec 2021 00:03:57 +0800
> Zhou Qingyang <zhou1615@umn.edu> escreveu:
> 
> > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> > to a NULL pointer dereference on failure of kzalloc().
> > 
> > I fix this bug by adding a NULL check of new_ts.
> > 
> > This bug was found by a static analyzer. The analysis employs
> > differential checking to identify inconsistent security operations
> > (e.g., checks or kfrees) between two code paths and confirms that the
> > inconsistent operations are not recovered in the current function or
> > the callers, so they constitute bugs.
> > 
> > Note that, as a bug found by static analysis, it can be a false
> > positive or hard to trigger. Multiple researchers have cross-reviewed
> > the bug.
> > 
> > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> > and our static analyzer no longer warns about this code.
> > 
> > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> > ---
> > Changes in v2:
> >   -  Delete dev_err() message
> > 
> >  drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> > index b9125c295d1d..ac60514c475b 100644
> > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> > @@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> >  	unsigned long flags;
> >  
> >  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> > +	if (!new_ts)
> > +		return;
> > +
> >  	new_ts->ts = ts;
> >  	new_ts->tc = tc;
> >  	new_ts->offset = offset;
> 
> I don't think this change is ok. Sure, it needs to check if
> kzalloc() fails, but it should return -ENOMEM and the caller
> should check if it returns an error. So, I would expect
> that this patch would also touch the caller function at
> drivers/staging/media/meson/vdec/esparser.c.

This is why umn.edu emails still are in my black-hole :(

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Greg Kroah-Hartman]

On Tue, Dec 14, 2021 at 02:46:13PM +0100, Mauro Carvalho Chehab wrote:
> Em Fri,  3 Dec 2021 00:03:57 +0800
> Zhou Qingyang <zhou1615@umn.edu> escreveu:
> 
> > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> > to a NULL pointer dereference on failure of kzalloc().
> > 
> > I fix this bug by adding a NULL check of new_ts.
> > 
> > This bug was found by a static analyzer. The analysis employs
> > differential checking to identify inconsistent security operations
> > (e.g., checks or kfrees) between two code paths and confirms that the
> > inconsistent operations are not recovered in the current function or
> > the callers, so they constitute bugs.
> > 
> > Note that, as a bug found by static analysis, it can be a false
> > positive or hard to trigger. Multiple researchers have cross-reviewed
> > the bug.
> > 
> > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> > and our static analyzer no longer warns about this code.
> > 
> > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> > ---
> > Changes in v2:
> >   -  Delete dev_err() message
> > 
> >  drivers/staging/media/meson/vdec/vdec_helpers.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> > index b9125c295d1d..ac60514c475b 100644
> > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> > @@ -234,6 +234,9 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> >  	unsigned long flags;
> >  
> >  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> > +	if (!new_ts)
> > +		return;
> > +
> >  	new_ts->ts = ts;
> >  	new_ts->tc = tc;
> >  	new_ts->offset = offset;
> 
> I don't think this change is ok. Sure, it needs to check if
> kzalloc() fails, but it should return -ENOMEM and the caller
> should check if it returns an error. So, I would expect
> that this patch would also touch the caller function at
> drivers/staging/media/meson/vdec/esparser.c.

This is why umn.edu emails still are in my black-hole :(

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

Fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer[1].

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---

[1] The analysis employs differential checking to identify inconsistent
security operations (e.g., checks or kfrees) between two code paths and
confirms that the inconsistent operations are not recovered in the 
current function or the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Changes in v3:
  -  Change the description of patch
  -  Turn the return type from 'void' to 'int'
  -  Check the return value in the caller 'esparser_queue()'

Changes in v2:
  -  Delete dev_err() message

 drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
 drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
 drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
index db7022707ff8..095100a50da8 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
 
 	offset = esparser_get_offset(sess);
 
-	amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+	ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+	if (!ret) {
+		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+		return ret;
+	}
+
 	dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
 		vb->timestamp, payload_size, offset, vbuf->flags);
 
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..06fd66539797 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
 }
 EXPORT_SYMBOL_GPL(amvdec_set_canvases);
 
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-		   struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+		  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
 {
 	struct amvdec_timestamp *new_ts;
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts)
+		return -ENOMEM;
+
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
@@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	spin_lock_irqsave(&sess->ts_spinlock, flags);
 	list_add_tail(&new_ts->list, &sess->timestamps);
 	spin_unlock_irqrestore(&sess->ts_spinlock, flags);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(amvdec_add_ts);
 
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
index 88137d15aa3a..4bf3e61d081b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
  * @offset: offset in the VIFIFO where the associated packet was written
  * @flags: the vb2_v4l2_buffer flags
  */
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-		   struct v4l2_timecode tc, u32 offset, u32 flags);
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+		  struct v4l2_timecode tc, u32 offset, u32 flags);
 void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
 
 /**
-- 
2.25.1

[PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

Fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer[1].

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---

[1] The analysis employs differential checking to identify inconsistent
security operations (e.g., checks or kfrees) between two code paths and
confirms that the inconsistent operations are not recovered in the 
current function or the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Changes in v3:
  -  Change the description of patch
  -  Turn the return type from 'void' to 'int'
  -  Check the return value in the caller 'esparser_queue()'

Changes in v2:
  -  Delete dev_err() message

 drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
 drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
 drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
index db7022707ff8..095100a50da8 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
 
 	offset = esparser_get_offset(sess);
 
-	amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+	ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+	if (!ret) {
+		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+		return ret;
+	}
+
 	dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
 		vb->timestamp, payload_size, offset, vbuf->flags);
 
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..06fd66539797 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
 }
 EXPORT_SYMBOL_GPL(amvdec_set_canvases);
 
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-		   struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+		  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
 {
 	struct amvdec_timestamp *new_ts;
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts)
+		return -ENOMEM;
+
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
@@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	spin_lock_irqsave(&sess->ts_spinlock, flags);
 	list_add_tail(&new_ts->list, &sess->timestamps);
 	spin_unlock_irqrestore(&sess->ts_spinlock, flags);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(amvdec_add_ts);
 
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
index 88137d15aa3a..4bf3e61d081b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
  * @offset: offset in the VIFIFO where the associated packet was written
  * @flags: the vb2_v4l2_buffer flags
  */
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-		   struct v4l2_timecode tc, u32 offset, u32 flags);
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+		  struct v4l2_timecode tc, u32 offset, u32 flags);
 void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
 
 /**
-- 
2.25.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Zhou Qingyang]

In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
to a NULL pointer dereference on failure of kzalloc().

Fix this bug by adding a NULL check of new_ts.

This bug was found by a static analyzer[1].

Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
and our static analyzer no longer warns about this code.

Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
---

[1] The analysis employs differential checking to identify inconsistent
security operations (e.g., checks or kfrees) between two code paths and
confirms that the inconsistent operations are not recovered in the 
current function or the callers, so they constitute bugs.

Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.

Changes in v3:
  -  Change the description of patch
  -  Turn the return type from 'void' to 'int'
  -  Check the return value in the caller 'esparser_queue()'

Changes in v2:
  -  Delete dev_err() message

 drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
 drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
 drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
index db7022707ff8..095100a50da8 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
 
 	offset = esparser_get_offset(sess);
 
-	amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+	ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+	if (!ret) {
+		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+		return ret;
+	}
+
 	dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
 		vb->timestamp, payload_size, offset, vbuf->flags);
 
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..06fd66539797 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
 }
 EXPORT_SYMBOL_GPL(amvdec_set_canvases);
 
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-		   struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+		  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
 {
 	struct amvdec_timestamp *new_ts;
 	unsigned long flags;
 
 	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+	if (!new_ts)
+		return -ENOMEM;
+
 	new_ts->ts = ts;
 	new_ts->tc = tc;
 	new_ts->offset = offset;
@@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
 	spin_lock_irqsave(&sess->ts_spinlock, flags);
 	list_add_tail(&new_ts->list, &sess->timestamps);
 	spin_unlock_irqrestore(&sess->ts_spinlock, flags);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(amvdec_add_ts);
 
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
index 88137d15aa3a..4bf3e61d081b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
  * @offset: offset in the VIFIFO where the associated packet was written
  * @flags: the vb2_v4l2_buffer flags
  */
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-		   struct v4l2_timecode tc, u32 offset, u32 flags);
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+		  struct v4l2_timecode tc, u32 offset, u32 flags);
 void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
 
 /**
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Hans Verkuil]

Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
name on it. Not nice.

Neil, can you post your patch with your own Signed-off-by, then I'll take that one.

Regards,

	Hans

On 15/12/2021 04:35, Zhou Qingyang wrote:
> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> Fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer[1].
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> 
> [1] The analysis employs differential checking to identify inconsistent
> security operations (e.g., checks or kfrees) between two code paths and
> confirms that the inconsistent operations are not recovered in the 
> current function or the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Changes in v3:
>   -  Change the description of patch
>   -  Turn the return type from 'void' to 'int'
>   -  Check the return value in the caller 'esparser_queue()'
> 
> Changes in v2:
>   -  Delete dev_err() message
> 
>  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index db7022707ff8..095100a50da8 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  
>  	offset = esparser_get_offset(sess);
>  
> -	amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
> +	ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
> +	if (!ret) {
> +		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> +		return ret;
> +	}
> +
>  	dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>  		vb->timestamp, payload_size, offset, vbuf->flags);
>  
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..06fd66539797 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>  }
>  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>  
> -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> -		   struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
> +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> +		  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>  {
>  	struct amvdec_timestamp *new_ts;
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts)
> +		return -ENOMEM;
> +
>  	new_ts->ts = ts;
>  	new_ts->tc = tc;
>  	new_ts->offset = offset;
> @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	spin_lock_irqsave(&sess->ts_spinlock, flags);
>  	list_add_tail(&new_ts->list, &sess->timestamps);
>  	spin_unlock_irqrestore(&sess->ts_spinlock, flags);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>  
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
> index 88137d15aa3a..4bf3e61d081b 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
> @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>   * @offset: offset in the VIFIFO where the associated packet was written
>   * @flags: the vb2_v4l2_buffer flags
>   */
> -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> -		   struct v4l2_timecode tc, u32 offset, u32 flags);
> +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> +		  struct v4l2_timecode tc, u32 offset, u32 flags);
>  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>  
>  /**

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Hans Verkuil]

Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
name on it. Not nice.

Neil, can you post your patch with your own Signed-off-by, then I'll take that one.

Regards,

	Hans

On 15/12/2021 04:35, Zhou Qingyang wrote:
> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> Fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer[1].
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> 
> [1] The analysis employs differential checking to identify inconsistent
> security operations (e.g., checks or kfrees) between two code paths and
> confirms that the inconsistent operations are not recovered in the 
> current function or the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Changes in v3:
>   -  Change the description of patch
>   -  Turn the return type from 'void' to 'int'
>   -  Check the return value in the caller 'esparser_queue()'
> 
> Changes in v2:
>   -  Delete dev_err() message
> 
>  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index db7022707ff8..095100a50da8 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  
>  	offset = esparser_get_offset(sess);
>  
> -	amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
> +	ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
> +	if (!ret) {
> +		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> +		return ret;
> +	}
> +
>  	dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>  		vb->timestamp, payload_size, offset, vbuf->flags);
>  
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..06fd66539797 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>  }
>  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>  
> -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> -		   struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
> +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> +		  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>  {
>  	struct amvdec_timestamp *new_ts;
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts)
> +		return -ENOMEM;
> +
>  	new_ts->ts = ts;
>  	new_ts->tc = tc;
>  	new_ts->offset = offset;
> @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	spin_lock_irqsave(&sess->ts_spinlock, flags);
>  	list_add_tail(&new_ts->list, &sess->timestamps);
>  	spin_unlock_irqrestore(&sess->ts_spinlock, flags);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>  
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
> index 88137d15aa3a..4bf3e61d081b 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
> @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>   * @offset: offset in the VIFIFO where the associated packet was written
>   * @flags: the vb2_v4l2_buffer flags
>   */
> -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> -		   struct v4l2_timecode tc, u32 offset, u32 flags);
> +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> +		  struct v4l2_timecode tc, u32 offset, u32 flags);
>  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>  
>  /**


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Hans Verkuil]

Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
name on it. Not nice.

Neil, can you post your patch with your own Signed-off-by, then I'll take that one.

Regards,

	Hans

On 15/12/2021 04:35, Zhou Qingyang wrote:
> In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
> to a NULL pointer dereference on failure of kzalloc().
> 
> Fix this bug by adding a NULL check of new_ts.
> 
> This bug was found by a static analyzer[1].
> 
> Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
> and our static analyzer no longer warns about this code.
> 
> Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> ---
> 
> [1] The analysis employs differential checking to identify inconsistent
> security operations (e.g., checks or kfrees) between two code paths and
> confirms that the inconsistent operations are not recovered in the 
> current function or the callers, so they constitute bugs.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
> 
> Changes in v3:
>   -  Change the description of patch
>   -  Turn the return type from 'void' to 'int'
>   -  Check the return value in the caller 'esparser_queue()'
> 
> Changes in v2:
>   -  Delete dev_err() message
> 
>  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index db7022707ff8..095100a50da8 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  
>  	offset = esparser_get_offset(sess);
>  
> -	amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
> +	ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
> +	if (!ret) {
> +		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> +		return ret;
> +	}
> +
>  	dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>  		vb->timestamp, payload_size, offset, vbuf->flags);
>  
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index b9125c295d1d..06fd66539797 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>  }
>  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>  
> -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> -		   struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
> +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> +		  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>  {
>  	struct amvdec_timestamp *new_ts;
>  	unsigned long flags;
>  
>  	new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
> +	if (!new_ts)
> +		return -ENOMEM;
> +
>  	new_ts->ts = ts;
>  	new_ts->tc = tc;
>  	new_ts->offset = offset;
> @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>  	spin_lock_irqsave(&sess->ts_spinlock, flags);
>  	list_add_tail(&new_ts->list, &sess->timestamps);
>  	spin_unlock_irqrestore(&sess->ts_spinlock, flags);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>  
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
> index 88137d15aa3a..4bf3e61d081b 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
> @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>   * @offset: offset in the VIFIFO where the associated packet was written
>   * @flags: the vb2_v4l2_buffer flags
>   */
> -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> -		   struct v4l2_timecode tc, u32 offset, u32 flags);
> +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
> +		  struct v4l2_timecode tc, u32 offset, u32 flags);
>  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>  
>  /**


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Hans Verkuil]

On 11/01/2022 14:03, Qingyang Zhou wrote:
> Hi Hans:
> 
> Here I certify that this is an origin patch I wrote. I did not copy Neil's patch, and I did not know his patch.
> 
> Neil, please post your original patch here, so everyone can see both patches.

OK, so I have three variants:

Yours:

https://patchwork.linuxtv.org/project/linux-media/patch/20211215033535.40422-1-zhou1615@umn.edu/

v2 from Jiasheng Jiang:

https://patchwork.linuxtv.org/project/linux-media/patch/20211210015620.2143555-1-jiasheng@iscas.ac.cn/

And v1 from Jiasheng Jiang which in the reply contains Neil's patch which is almost identical to
both Jiasheng's v2 and your v3 (it's just missing a return 0).

https://patchwork.linuxtv.org/project/linux-media/patch/20211209085840.2081024-1-jiasheng@iscas.ac.cn/

So this is a case of parallel evolution. But regardless, I do prefer it if Neil can
post the patch: he's the driver maintainer, so I'd be happiest with a patch from him.

Regards,

	Hans

> 
> Yours 
> Zhou Qingyang.
> 
> On Tue, Jan 11, 2022 at 5:16 PM Hans Verkuil <hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>> wrote:
> 
>     Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
>     name on it. Not nice.
> 
>     Neil, can you post your patch with your own Signed-off-by, then I'll take that one.
> 
>     Regards,
> 
>             Hans
> 
>     On 15/12/2021 04:35, Zhou Qingyang wrote:
>     > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
>     > to a NULL pointer dereference on failure of kzalloc().
>     >
>     > Fix this bug by adding a NULL check of new_ts.
>     >
>     > This bug was found by a static analyzer[1].
>     >
>     > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
>     > and our static analyzer no longer warns about this code.
>     >
>     > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
>     > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu <mailto:zhou1615@umn.edu>>
>     > ---
>     >
>     > [1] The analysis employs differential checking to identify inconsistent
>     > security operations (e.g., checks or kfrees) between two code paths and
>     > confirms that the inconsistent operations are not recovered in the
>     > current function or the callers, so they constitute bugs.
>     >
>     > Note that, as a bug found by static analysis, it can be a false
>     > positive or hard to trigger. Multiple researchers have cross-reviewed
>     > the bug.
>     >
>     > Changes in v3:
>     >   -  Change the description of patch
>     >   -  Turn the return type from 'void' to 'int'
>     >   -  Check the return value in the caller 'esparser_queue()'
>     >
>     > Changes in v2:
>     >   -  Delete dev_err() message
>     >
>     >  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>     >  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>     >  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>     >  3 files changed, 14 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
>     > index db7022707ff8..095100a50da8 100644
>     > --- a/drivers/staging/media/meson/vdec/esparser.c
>     > +++ b/drivers/staging/media/meson/vdec/esparser.c
>     > @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>     > 
>     >       offset = esparser_get_offset(sess);
>     > 
>     > -     amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     if (!ret) {
>     > +             v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>     > +             return ret;
>     > +     }
>     > +
>     >       dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>     >               vb->timestamp, payload_size, offset, vbuf->flags);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > index b9125c295d1d..06fd66539797 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>     > 
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     >  {
>     >       struct amvdec_timestamp *new_ts;
>     >       unsigned long flags;
>     > 
>     >       new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
>     > +     if (!new_ts)
>     > +             return -ENOMEM;
>     > +
>     >       new_ts->ts = ts;
>     >       new_ts->tc = tc;
>     >       new_ts->offset = offset;
>     > @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     >       spin_lock_irqsave(&sess->ts_spinlock, flags);
>     >       list_add_tail(&new_ts->list, &sess->timestamps);
>     >       spin_unlock_irqrestore(&sess->ts_spinlock, flags);
>     > +     return 0;
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > index 88137d15aa3a..4bf3e61d081b 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>     >   * @offset: offset in the VIFIFO where the associated packet was written
>     >   * @flags: the vb2_v4l2_buffer flags
>     >   */
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 flags);
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 flags);
>     >  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>     > 
>     >  /**
> 

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Neil Armstrong]

Hi,

On 11/01/2022 14:03, Qingyang Zhou wrote:
> Hi Hans:
> 
> Here I certify that this is an origin patch I wrote. I did not copy Neil's patch, and I did not know his patch.
> 
> Neil, please post your original patch here, so everyone can see both patches.
> 
> Yours 
> Zhou Qingyang.
> 
> On Tue, Jan 11, 2022 at 5:16 PM Hans Verkuil <hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>> wrote:
> 
>     Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
>     name on it. Not nice.
> 
>     Neil, can you post your patch with your own Signed-off-by, then I'll take that one.
> 
>     Regards,
> 
>             Hans
> 
>     On 15/12/2021 04:35, Zhou Qingyang wrote:
>     > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
>     > to a NULL pointer dereference on failure of kzalloc().
>     >
>     > Fix this bug by adding a NULL check of new_ts.
>     >
>     > This bug was found by a static analyzer[1].
>     >
>     > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
>     > and our static analyzer no longer warns about this code.
>     >
>     > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
>     > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu <mailto:zhou1615@umn.edu>>
>     > ---
>     >
>     > [1] The analysis employs differential checking to identify inconsistent
>     > security operations (e.g., checks or kfrees) between two code paths and
>     > confirms that the inconsistent operations are not recovered in the
>     > current function or the callers, so they constitute bugs.
>     >
>     > Note that, as a bug found by static analysis, it can be a false
>     > positive or hard to trigger. Multiple researchers have cross-reviewed
>     > the bug.
>     >
>     > Changes in v3:
>     >   -  Change the description of patch
>     >   -  Turn the return type from 'void' to 'int'
>     >   -  Check the return value in the caller 'esparser_queue()'
>     >
>     > Changes in v2:
>     >   -  Delete dev_err() message
>     >
>     >  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>     >  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>     >  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>     >  3 files changed, 14 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
>     > index db7022707ff8..095100a50da8 100644
>     > --- a/drivers/staging/media/meson/vdec/esparser.c
>     > +++ b/drivers/staging/media/meson/vdec/esparser.c
>     > @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>     > 
>     >       offset = esparser_get_offset(sess);
>     > 
>     > -     amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     if (!ret) {
>     > +             v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>     > +             return ret;
>     > +     }
>     > +
>     >       dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>     >               vb->timestamp, payload_size, offset, vbuf->flags);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > index b9125c295d1d..06fd66539797 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>     > 
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     >  {
>     >       struct amvdec_timestamp *new_ts;
>     >       unsigned long flags;
>     > 
>     >       new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
>     > +     if (!new_ts)
>     > +             return -ENOMEM;
>     > +
>     >       new_ts->ts = ts;
>     >       new_ts->tc = tc;
>     >       new_ts->offset = offset;
>     > @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     >       spin_lock_irqsave(&sess->ts_spinlock, flags);
>     >       list_add_tail(&new_ts->list, &sess->timestamps);
>     >       spin_unlock_irqrestore(&sess->ts_spinlock, flags);
>     > +     return 0;
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > index 88137d15aa3a..4bf3e61d081b 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>     >   * @offset: offset in the VIFIFO where the associated packet was written
>     >   * @flags: the vb2_v4l2_buffer flags
>     >   */
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 flags);
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 flags);
>     >  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>     > 
>     >  /**
> 

Let's inspect:

I posted a probable solution the 09/12/2021 in a reply to 20211209085840.2081024-1-jiasheng@iscas.ac.cn :

===================><=============================
diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
index db7022707ff8..df733eff9ac7 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -328,7 +328,11 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)

        offset = esparser_get_offset(sess);

-       amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+       ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+       if (ret) {
+               v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+               return ret;
+       }
        dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
                vb->timestamp, payload_size, offset, vbuf->flags);

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..593b2ccbece2 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -227,13 +227,15 @@ int amvdec_set_canvases(struct amvdec_session *sess,
 }
 EXPORT_SYMBOL_GPL(amvdec_set_canvases);

-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-                  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+                 struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
 {
        struct amvdec_timestamp *new_ts;
        unsigned long flags;

        new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+       if (!new_ts)
+               return -ENOMEM;
        new_ts->ts = ts;
        new_ts->tc = tc;
        new_ts->offset = offset;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
index 88137d15aa3a..4bf3e61d081b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
  * @offset: offset in the VIFIFO where the associated packet was written
  * @flags: the vb2_v4l2_buffer flags
  */
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-                  struct v4l2_timecode tc, u32 offset, u32 flags);
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+                 struct v4l2_timecode tc, u32 offset, u32 flags);
 void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
===================><=============================


Then the patches were sent:
- 20211210015620.2143555-1-jiasheng@iscas.ac.cn on 10/12/2021
- 20211215033535.40422-1-zhou1615@umn.edu on 15/12/2021

They are extremely close but not similar, mostly indenting differs.

Both patches have the missing final "return 0" in amvdec_add_ts which is missing in my proposal.

But only 20211210015620.2143555-1-jiasheng@iscas.ac.cn has the correct "if (ret)" in esparser_queue().
Patch 20211215033535.40422-1-zhou1615@umn.ed has a wrong "if (!ret)".

But when comparing, 20211215033535.40422-1-zhou1615@umn.edu is an almost exact copy of my proposal, minus the fixes and the bogus return check.

To be honest, there is a limited way to fix this, it's probable 20211215033535.40422-1-zhou1615@umn.edu was written independently from
my proposal since there is a bug return check, and 20211210015620.2143555-1-jiasheng@iscas.ac.cn was rewritten from my proposal.

Since 20211215033535.40422-1-zhou1615@umn.ed has a bogus return check, it should be naked.

I'll only ask 20211210015620.2143555-1-jiasheng@iscas.ac.cn to be resent with a "Suggested-by" and indentation fixed like my proposal.

Neil

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Neil Armstrong]

Hi,

On 11/01/2022 14:03, Qingyang Zhou wrote:
> Hi Hans:
> 
> Here I certify that this is an origin patch I wrote. I did not copy Neil's patch, and I did not know his patch.
> 
> Neil, please post your original patch here, so everyone can see both patches.
> 
> Yours 
> Zhou Qingyang.
> 
> On Tue, Jan 11, 2022 at 5:16 PM Hans Verkuil <hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>> wrote:
> 
>     Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
>     name on it. Not nice.
> 
>     Neil, can you post your patch with your own Signed-off-by, then I'll take that one.
> 
>     Regards,
> 
>             Hans
> 
>     On 15/12/2021 04:35, Zhou Qingyang wrote:
>     > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
>     > to a NULL pointer dereference on failure of kzalloc().
>     >
>     > Fix this bug by adding a NULL check of new_ts.
>     >
>     > This bug was found by a static analyzer[1].
>     >
>     > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
>     > and our static analyzer no longer warns about this code.
>     >
>     > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
>     > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu <mailto:zhou1615@umn.edu>>
>     > ---
>     >
>     > [1] The analysis employs differential checking to identify inconsistent
>     > security operations (e.g., checks or kfrees) between two code paths and
>     > confirms that the inconsistent operations are not recovered in the
>     > current function or the callers, so they constitute bugs.
>     >
>     > Note that, as a bug found by static analysis, it can be a false
>     > positive or hard to trigger. Multiple researchers have cross-reviewed
>     > the bug.
>     >
>     > Changes in v3:
>     >   -  Change the description of patch
>     >   -  Turn the return type from 'void' to 'int'
>     >   -  Check the return value in the caller 'esparser_queue()'
>     >
>     > Changes in v2:
>     >   -  Delete dev_err() message
>     >
>     >  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>     >  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>     >  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>     >  3 files changed, 14 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
>     > index db7022707ff8..095100a50da8 100644
>     > --- a/drivers/staging/media/meson/vdec/esparser.c
>     > +++ b/drivers/staging/media/meson/vdec/esparser.c
>     > @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>     > 
>     >       offset = esparser_get_offset(sess);
>     > 
>     > -     amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     if (!ret) {
>     > +             v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>     > +             return ret;
>     > +     }
>     > +
>     >       dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>     >               vb->timestamp, payload_size, offset, vbuf->flags);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > index b9125c295d1d..06fd66539797 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>     > 
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     >  {
>     >       struct amvdec_timestamp *new_ts;
>     >       unsigned long flags;
>     > 
>     >       new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
>     > +     if (!new_ts)
>     > +             return -ENOMEM;
>     > +
>     >       new_ts->ts = ts;
>     >       new_ts->tc = tc;
>     >       new_ts->offset = offset;
>     > @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     >       spin_lock_irqsave(&sess->ts_spinlock, flags);
>     >       list_add_tail(&new_ts->list, &sess->timestamps);
>     >       spin_unlock_irqrestore(&sess->ts_spinlock, flags);
>     > +     return 0;
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > index 88137d15aa3a..4bf3e61d081b 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>     >   * @offset: offset in the VIFIFO where the associated packet was written
>     >   * @flags: the vb2_v4l2_buffer flags
>     >   */
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 flags);
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 flags);
>     >  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>     > 
>     >  /**
> 

Let's inspect:

I posted a probable solution the 09/12/2021 in a reply to 20211209085840.2081024-1-jiasheng@iscas.ac.cn :

===================><=============================
diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
index db7022707ff8..df733eff9ac7 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -328,7 +328,11 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)

        offset = esparser_get_offset(sess);

-       amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+       ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+       if (ret) {
+               v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+               return ret;
+       }
        dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
                vb->timestamp, payload_size, offset, vbuf->flags);

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..593b2ccbece2 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -227,13 +227,15 @@ int amvdec_set_canvases(struct amvdec_session *sess,
 }
 EXPORT_SYMBOL_GPL(amvdec_set_canvases);

-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-                  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+                 struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
 {
        struct amvdec_timestamp *new_ts;
        unsigned long flags;

        new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+       if (!new_ts)
+               return -ENOMEM;
        new_ts->ts = ts;
        new_ts->tc = tc;
        new_ts->offset = offset;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
index 88137d15aa3a..4bf3e61d081b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
  * @offset: offset in the VIFIFO where the associated packet was written
  * @flags: the vb2_v4l2_buffer flags
  */
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-                  struct v4l2_timecode tc, u32 offset, u32 flags);
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+                 struct v4l2_timecode tc, u32 offset, u32 flags);
 void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
===================><=============================


Then the patches were sent:
- 20211210015620.2143555-1-jiasheng@iscas.ac.cn on 10/12/2021
- 20211215033535.40422-1-zhou1615@umn.edu on 15/12/2021

They are extremely close but not similar, mostly indenting differs.

Both patches have the missing final "return 0" in amvdec_add_ts which is missing in my proposal.

But only 20211210015620.2143555-1-jiasheng@iscas.ac.cn has the correct "if (ret)" in esparser_queue().
Patch 20211215033535.40422-1-zhou1615@umn.ed has a wrong "if (!ret)".

But when comparing, 20211215033535.40422-1-zhou1615@umn.edu is an almost exact copy of my proposal, minus the fixes and the bogus return check.

To be honest, there is a limited way to fix this, it's probable 20211215033535.40422-1-zhou1615@umn.edu was written independently from
my proposal since there is a bug return check, and 20211210015620.2143555-1-jiasheng@iscas.ac.cn was rewritten from my proposal.

Since 20211215033535.40422-1-zhou1615@umn.ed has a bogus return check, it should be naked.

I'll only ask 20211210015620.2143555-1-jiasheng@iscas.ac.cn to be resent with a "Suggested-by" and indentation fixed like my proposal.

Neil


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Neil Armstrong]

Hi,

On 11/01/2022 14:03, Qingyang Zhou wrote:
> Hi Hans:
> 
> Here I certify that this is an origin patch I wrote. I did not copy Neil's patch, and I did not know his patch.
> 
> Neil, please post your original patch here, so everyone can see both patches.
> 
> Yours 
> Zhou Qingyang.
> 
> On Tue, Jan 11, 2022 at 5:16 PM Hans Verkuil <hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>> wrote:
> 
>     Zhou Qingyang, this is exactly the patch Neil wrote, except you just stuck your
>     name on it. Not nice.
> 
>     Neil, can you post your patch with your own Signed-off-by, then I'll take that one.
> 
>     Regards,
> 
>             Hans
> 
>     On 15/12/2021 04:35, Zhou Qingyang wrote:
>     > In amvdec_add_ts(), there is a dereference of kzalloc(), which could lead
>     > to a NULL pointer dereference on failure of kzalloc().
>     >
>     > Fix this bug by adding a NULL check of new_ts.
>     >
>     > This bug was found by a static analyzer[1].
>     >
>     > Builds with CONFIG_VIDEO_MESON_VDEC=m show no new warnings,
>     > and our static analyzer no longer warns about this code.
>     >
>     > Fixes: 876f123b8956 ("media: meson: vdec: bring up to compliance")
>     > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu <mailto:zhou1615@umn.edu>>
>     > ---
>     >
>     > [1] The analysis employs differential checking to identify inconsistent
>     > security operations (e.g., checks or kfrees) between two code paths and
>     > confirms that the inconsistent operations are not recovered in the
>     > current function or the callers, so they constitute bugs.
>     >
>     > Note that, as a bug found by static analysis, it can be a false
>     > positive or hard to trigger. Multiple researchers have cross-reviewed
>     > the bug.
>     >
>     > Changes in v3:
>     >   -  Change the description of patch
>     >   -  Turn the return type from 'void' to 'int'
>     >   -  Check the return value in the caller 'esparser_queue()'
>     >
>     > Changes in v2:
>     >   -  Delete dev_err() message
>     >
>     >  drivers/staging/media/meson/vdec/esparser.c     | 7 ++++++-
>     >  drivers/staging/media/meson/vdec/vdec_helpers.c | 8 ++++++--
>     >  drivers/staging/media/meson/vdec/vdec_helpers.h | 4 ++--
>     >  3 files changed, 14 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
>     > index db7022707ff8..095100a50da8 100644
>     > --- a/drivers/staging/media/meson/vdec/esparser.c
>     > +++ b/drivers/staging/media/meson/vdec/esparser.c
>     > @@ -328,7 +328,12 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>     > 
>     >       offset = esparser_get_offset(sess);
>     > 
>     > -     amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
>     > +     if (!ret) {
>     > +             v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>     > +             return ret;
>     > +     }
>     > +
>     >       dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
>     >               vb->timestamp, payload_size, offset, vbuf->flags);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > index b9125c295d1d..06fd66539797 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
>     > @@ -227,13 +227,16 @@ int amvdec_set_canvases(struct amvdec_session *sess,
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_set_canvases);
>     > 
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
>     >  {
>     >       struct amvdec_timestamp *new_ts;
>     >       unsigned long flags;
>     > 
>     >       new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
>     > +     if (!new_ts)
>     > +             return -ENOMEM;
>     > +
>     >       new_ts->ts = ts;
>     >       new_ts->tc = tc;
>     >       new_ts->offset = offset;
>     > @@ -242,6 +245,7 @@ void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     >       spin_lock_irqsave(&sess->ts_spinlock, flags);
>     >       list_add_tail(&new_ts->list, &sess->timestamps);
>     >       spin_unlock_irqrestore(&sess->ts_spinlock, flags);
>     > +     return 0;
>     >  }
>     >  EXPORT_SYMBOL_GPL(amvdec_add_ts);
>     > 
>     > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > index 88137d15aa3a..4bf3e61d081b 100644
>     > --- a/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
>     > @@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
>     >   * @offset: offset in the VIFIFO where the associated packet was written
>     >   * @flags: the vb2_v4l2_buffer flags
>     >   */
>     > -void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > -                struct v4l2_timecode tc, u32 offset, u32 flags);
>     > +int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
>     > +               struct v4l2_timecode tc, u32 offset, u32 flags);
>     >  void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
>     > 
>     >  /**
> 

Let's inspect:

I posted a probable solution the 09/12/2021 in a reply to 20211209085840.2081024-1-jiasheng@iscas.ac.cn :

===================><=============================
diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
index db7022707ff8..df733eff9ac7 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -328,7 +328,11 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)

        offset = esparser_get_offset(sess);

-       amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+       ret = amvdec_add_ts(sess, vb->timestamp, vbuf->timecode, offset, vbuf->flags);
+       if (ret) {
+               v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
+               return ret;
+       }
        dev_dbg(core->dev, "esparser: ts = %llu pld_size = %u offset = %08X flags = %08X\n",
                vb->timestamp, payload_size, offset, vbuf->flags);

diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
index b9125c295d1d..593b2ccbece2 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -227,13 +227,15 @@ int amvdec_set_canvases(struct amvdec_session *sess,
 }
 EXPORT_SYMBOL_GPL(amvdec_set_canvases);

-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-                  struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+                 struct v4l2_timecode tc, u32 offset, u32 vbuf_flags)
 {
        struct amvdec_timestamp *new_ts;
        unsigned long flags;

        new_ts = kzalloc(sizeof(*new_ts), GFP_KERNEL);
+       if (!new_ts)
+               return -ENOMEM;
        new_ts->ts = ts;
        new_ts->tc = tc;
        new_ts->offset = offset;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h b/drivers/staging/media/meson/vdec/vdec_helpers.h
index 88137d15aa3a..4bf3e61d081b 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -56,8 +56,8 @@ void amvdec_dst_buf_done_offset(struct amvdec_session *sess,
  * @offset: offset in the VIFIFO where the associated packet was written
  * @flags: the vb2_v4l2_buffer flags
  */
-void amvdec_add_ts(struct amvdec_session *sess, u64 ts,
-                  struct v4l2_timecode tc, u32 offset, u32 flags);
+int amvdec_add_ts(struct amvdec_session *sess, u64 ts,
+                 struct v4l2_timecode tc, u32 offset, u32 flags);
 void amvdec_remove_ts(struct amvdec_session *sess, u64 ts);
===================><=============================


Then the patches were sent:
- 20211210015620.2143555-1-jiasheng@iscas.ac.cn on 10/12/2021
- 20211215033535.40422-1-zhou1615@umn.edu on 15/12/2021

They are extremely close but not similar, mostly indenting differs.

Both patches have the missing final "return 0" in amvdec_add_ts which is missing in my proposal.

But only 20211210015620.2143555-1-jiasheng@iscas.ac.cn has the correct "if (ret)" in esparser_queue().
Patch 20211215033535.40422-1-zhou1615@umn.ed has a wrong "if (!ret)".

But when comparing, 20211215033535.40422-1-zhou1615@umn.edu is an almost exact copy of my proposal, minus the fixes and the bogus return check.

To be honest, there is a limited way to fix this, it's probable 20211215033535.40422-1-zhou1615@umn.edu was written independently from
my proposal since there is a bug return check, and 20211210015620.2143555-1-jiasheng@iscas.ac.cn was rewritten from my proposal.

Since 20211215033535.40422-1-zhou1615@umn.ed has a bogus return check, it should be naked.

I'll only ask 20211210015620.2143555-1-jiasheng@iscas.ac.cn to be resent with a "Suggested-by" and indentation fixed like my proposal.

Neil


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] media: meson: vdec: Fix a NULL pointer dereference in amvdec_add_ts()

[Hans Verkuil]

Hi all,

On 12/01/2022 09:57, Neil Armstrong wrote:

<snip>

> 
> Then the patches were sent:
> - 20211210015620.2143555-1-jiasheng@iscas.ac.cn on 10/12/2021
> - 20211215033535.40422-1-zhou1615@umn.edu on 15/12/2021
> 
> They are extremely close but not similar, mostly indenting differs.
> 
> Both patches have the missing final "return 0" in amvdec_add_ts which is missing in my proposal.
> 
> But only 20211210015620.2143555-1-jiasheng@iscas.ac.cn has the correct "if (ret)" in esparser_queue().
> Patch 20211215033535.40422-1-zhou1615@umn.ed has a wrong "if (!ret)".
> 
> But when comparing, 20211215033535.40422-1-zhou1615@umn.edu is an almost exact copy of my proposal, minus the fixes and the bogus return check.
> 
> To be honest, there is a limited way to fix this, it's probable 20211215033535.40422-1-zhou1615@umn.edu was written independently from
> my proposal since there is a bug return check, and 20211210015620.2143555-1-jiasheng@iscas.ac.cn was rewritten from my proposal.
> 
> Since 20211215033535.40422-1-zhou1615@umn.ed has a bogus return check, it should be naked.

OK, I had already rejected this, so that remain rejected.

> 
> I'll only ask 20211210015620.2143555-1-jiasheng@iscas.ac.cn to be resent with a "Suggested-by" and indentation fixed like my proposal.

I changed the status of this patch to 'Changes Requested'. Jiasheng, can you post a v3 with
the requested changes?

Neil, thank you for your detailed analysis!

Regards,

	Hans