All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yong Wu <yong.wu@mediatek.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: kernel test robot <lkp@intel.com>,
	linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
	linux-mediatek@lists.infradead.org,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [SPAM][PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek,larbs"
Date: Tue, 14 Dec 2021 15:31:25 +0800	[thread overview]
Message-ID: <ebf58066a131f92c68e83a1ef56b88f435fa0d08.camel@mediatek.com> (raw)
In-Reply-To: <20211210205704.1664928-1-linux@roeck-us.net>

On Fri, 2021-12-10 at 12:57 -0800, Guenter Roeck wrote:
> Since commit baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
> common
> and m4u"), the driver assumes that at least one phandle associated
> with
> "mediatek,larbs" exists. If that is not the case, for example if
> reason
> "mediatek,larbs" is provided as boolean property, the code will use
> an
> uninitialized pointer and may crash. To fix the problem, ensure that
> the
> number of phandles associated with "mediatek,larbs" is at least 1 and
> bail out immediately if that is not the case.

From the dt-binding, "mediatek,larbs" always is a phandle-array. I 
assumed the dts should conform to the dt-binding before. Then the
problem is that if we should cover the case that someone abuses/attacks
the dts. Could you help add more comment in the commit message?
something like: this is for avoid abuse the dt-binding.

> 
> Cc: Yong Wu <yong.wu@mediatek.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-common
> and m4u")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/iommu/mtk_iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 25b834104790..0bbe32d0a2a6 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -828,6 +828,8 @@ static int mtk_iommu_probe(struct platform_device
> *pdev)
>  					     "mediatek,larbs", NULL);
>  	if (larb_nr < 0)
>  		return larb_nr;
> +	if (larb_nr == 0)
> +		return -EINVAL;

Just assigning the larbnode to NULL may be simpler. In this case, it
won't enter the loop below, and return 0 in the
of_parse_phandle(larbnode, "mediatek,smi", 0).

-       struct device_node      *larbnode, *smicomm_node;
+       struct device_node      *larbnode = NULL, *smicomm_node;

>  
>  	for (i = 0; i < larb_nr; i++) {
>  		u32 id;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

WARNING: multiple messages have this Message-ID (diff)
From: Yong Wu <yong.wu@mediatek.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	<iommu@lists.linux-foundation.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>,
	 <linux-kernel@vger.kernel.org>, Tomasz Figa <tfiga@chromium.org>,
	"kernel test robot" <lkp@intel.com>,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [SPAM][PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek,larbs"
Date: Tue, 14 Dec 2021 15:31:25 +0800	[thread overview]
Message-ID: <ebf58066a131f92c68e83a1ef56b88f435fa0d08.camel@mediatek.com> (raw)
In-Reply-To: <20211210205704.1664928-1-linux@roeck-us.net>

On Fri, 2021-12-10 at 12:57 -0800, Guenter Roeck wrote:
> Since commit baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
> common
> and m4u"), the driver assumes that at least one phandle associated
> with
> "mediatek,larbs" exists. If that is not the case, for example if
> reason
> "mediatek,larbs" is provided as boolean property, the code will use
> an
> uninitialized pointer and may crash. To fix the problem, ensure that
> the
> number of phandles associated with "mediatek,larbs" is at least 1 and
> bail out immediately if that is not the case.

From the dt-binding, "mediatek,larbs" always is a phandle-array. I 
assumed the dts should conform to the dt-binding before. Then the
problem is that if we should cover the case that someone abuses/attacks
the dts. Could you help add more comment in the commit message?
something like: this is for avoid abuse the dt-binding.

> 
> Cc: Yong Wu <yong.wu@mediatek.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-common
> and m4u")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/iommu/mtk_iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 25b834104790..0bbe32d0a2a6 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -828,6 +828,8 @@ static int mtk_iommu_probe(struct platform_device
> *pdev)
>  					     "mediatek,larbs", NULL);
>  	if (larb_nr < 0)
>  		return larb_nr;
> +	if (larb_nr == 0)
> +		return -EINVAL;

Just assigning the larbnode to NULL may be simpler. In this case, it
won't enter the loop below, and return 0 in the
of_parse_phandle(larbnode, "mediatek,smi", 0).

-       struct device_node      *larbnode, *smicomm_node;
+       struct device_node      *larbnode = NULL, *smicomm_node;

>  
>  	for (i = 0; i < larb_nr; i++) {
>  		u32 id;
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

WARNING: multiple messages have this Message-ID (diff)
From: Yong Wu <yong.wu@mediatek.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	<iommu@lists.linux-foundation.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>,
	 <linux-kernel@vger.kernel.org>, Tomasz Figa <tfiga@chromium.org>,
	"kernel test robot" <lkp@intel.com>,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [SPAM][PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek,larbs"
Date: Tue, 14 Dec 2021 15:31:25 +0800	[thread overview]
Message-ID: <ebf58066a131f92c68e83a1ef56b88f435fa0d08.camel@mediatek.com> (raw)
In-Reply-To: <20211210205704.1664928-1-linux@roeck-us.net>

On Fri, 2021-12-10 at 12:57 -0800, Guenter Roeck wrote:
> Since commit baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
> common
> and m4u"), the driver assumes that at least one phandle associated
> with
> "mediatek,larbs" exists. If that is not the case, for example if
> reason
> "mediatek,larbs" is provided as boolean property, the code will use
> an
> uninitialized pointer and may crash. To fix the problem, ensure that
> the
> number of phandles associated with "mediatek,larbs" is at least 1 and
> bail out immediately if that is not the case.

From the dt-binding, "mediatek,larbs" always is a phandle-array. I 
assumed the dts should conform to the dt-binding before. Then the
problem is that if we should cover the case that someone abuses/attacks
the dts. Could you help add more comment in the commit message?
something like: this is for avoid abuse the dt-binding.

> 
> Cc: Yong Wu <yong.wu@mediatek.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-common
> and m4u")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/iommu/mtk_iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 25b834104790..0bbe32d0a2a6 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -828,6 +828,8 @@ static int mtk_iommu_probe(struct platform_device
> *pdev)
>  					     "mediatek,larbs", NULL);
>  	if (larb_nr < 0)
>  		return larb_nr;
> +	if (larb_nr == 0)
> +		return -EINVAL;

Just assigning the larbnode to NULL may be simpler. In this case, it
won't enter the loop below, and return 0 in the
of_parse_phandle(larbnode, "mediatek,smi", 0).

-       struct device_node      *larbnode, *smicomm_node;
+       struct device_node      *larbnode = NULL, *smicomm_node;

>  
>  	for (i = 0; i < larb_nr; i++) {
>  		u32 id;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-12-14  7:31 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-10 20:57 [PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek,larbs" Guenter Roeck
2021-12-10 20:57 ` [PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek, larbs" Guenter Roeck
2021-12-10 20:57 ` Guenter Roeck
2021-12-10 20:57 ` Guenter Roeck
2021-12-14  7:31 ` Yong Wu [this message]
2021-12-14  7:31   ` [SPAM][PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek,larbs" Yong Wu
2021-12-14  7:31   ` Yong Wu
2021-12-14  9:04   ` Tzung-Bi Shih
2021-12-14  9:04     ` Tzung-Bi Shih via iommu
2021-12-14  9:04     ` Tzung-Bi Shih
2021-12-15  5:31     ` Yong Wu
2021-12-15  5:31       ` Yong Wu
2021-12-15  5:31       ` Yong Wu
2021-12-14 15:02   ` Guenter Roeck
2021-12-14 15:02     ` Guenter Roeck
2021-12-14 15:02     ` Guenter Roeck
2021-12-15  5:30     ` Yong Wu
2021-12-15  5:30       ` Yong Wu
2021-12-15  5:30       ` Yong Wu
2021-12-15 16:25       ` Guenter Roeck
2021-12-15 16:25         ` Guenter Roeck
2021-12-15 16:25         ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ebf58066a131f92c68e83a1ef56b88f435fa0d08.camel@mediatek.com \
    --to=yong.wu@mediatek.com \
    --cc=dan.carpenter@oracle.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux@roeck-us.net \
    --cc=lkp@intel.com \
    --cc=matthias.bgg@gmail.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.