[PATCH v2] mfd: ls2kbmc: mfd: ls2kbmc: Fix iomem pointer handling in video mode parsing

[PATCH v2] mfd: ls2kbmc: mfd: ls2kbmc: Fix iomem pointer handling in video mode parsing

[Binbin Zhou]

Use pointers annotated with the __iomem marker for all iomem map calls,
and creates a local copy of the mapped IO memory for future access in
the code. memcpy_fromio() and memcpy_toio() are used to read/write data
from/to mapped IO memory

Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202603021730.Yy3QXYTw-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202606120639.WG6eb8VU-lkp@intel.com/
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
V2:
 - Add the missing memcpy_fromio();
 - Drop the unnecessary `buf` variable.

Link to V1:
https://lore.kernel.org/all/20260616115530.4012675-1-zhoubinbin@loongson.cn/

 drivers/mfd/ls2k-bmc-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
index 408056bfb2fe..1312812860c0 100644
--- a/drivers/mfd/ls2k-bmc-core.c
+++ b/drivers/mfd/ls2k-bmc-core.c
@@ -427,14 +427,20 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
  */
 static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
 {
-	char *mode;
+	char *mode __free(kfree) = NULL;
+	void __iomem *base;
 	int depth, ret;
 
 	/* The last 16M of PCI BAR0 is used to store the resolution string. */
-	mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
+	base = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
+	if (!base)
+		return -ENOMEM;
+
+	mode = kmalloc(SZ_16M, GFP_KERNEL);
 	if (!mode)
 		return -ENOMEM;
 
+	memcpy_fromio(mode, base, SZ_16M);
 	/* The resolution field starts with the flag "video=". */
 	if (!strncmp(mode, "video=", 6))
 		mode = mode + 6;

base-commit: c454531af72e0df811600601413bb8d3d039ed08
-- 
2.52.0

Re: [PATCH v2] mfd: ls2kbmc: mfd: ls2kbmc: Fix iomem pointer handling in video mode parsing

[Huacai Chen]

Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>

On Wed, Jun 24, 2026 at 4:56 PM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
>
> Use pointers annotated with the __iomem marker for all iomem map calls,
> and creates a local copy of the mapped IO memory for future access in
> the code. memcpy_fromio() and memcpy_toio() are used to read/write data
> from/to mapped IO memory
>
> Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603021730.Yy3QXYTw-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202606120639.WG6eb8VU-lkp@intel.com/
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
> V2:
>  - Add the missing memcpy_fromio();
>  - Drop the unnecessary `buf` variable.
>
> Link to V1:
> https://lore.kernel.org/all/20260616115530.4012675-1-zhoubinbin@loongson.cn/
>
>  drivers/mfd/ls2k-bmc-core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe..1312812860c0 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -427,14 +427,20 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>   */
>  static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
>  {
> -       char *mode;
> +       char *mode __free(kfree) = NULL;
> +       void __iomem *base;
>         int depth, ret;
>
>         /* The last 16M of PCI BAR0 is used to store the resolution string. */
> -       mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> +       base = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> +       if (!base)
> +               return -ENOMEM;
> +
> +       mode = kmalloc(SZ_16M, GFP_KERNEL);
>         if (!mode)
>                 return -ENOMEM;
>
> +       memcpy_fromio(mode, base, SZ_16M);
>         /* The resolution field starts with the flag "video=". */
>         if (!strncmp(mode, "video=", 6))
>                 mode = mode + 6;
>
> base-commit: c454531af72e0df811600601413bb8d3d039ed08
> --
> 2.52.0
>

Re: [PATCH v2] mfd: ls2kbmc: mfd: ls2kbmc: Fix iomem pointer handling in video mode parsing

[Lee Jones]

Please consider these:

/* Sashiko Automation: Issues Found (4 Findings) */

On Wed, 24 Jun 2026, Binbin Zhou wrote:

> Use pointers annotated with the __iomem marker for all iomem map calls,
> and creates a local copy of the mapped IO memory for future access in
> the code. memcpy_fromio() and memcpy_toio() are used to read/write data
> from/to mapped IO memory
> 
> Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603021730.Yy3QXYTw-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202606120639.WG6eb8VU-lkp@intel.com/
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
> V2:
>  - Add the missing memcpy_fromio();
>  - Drop the unnecessary `buf` variable.
> 
> Link to V1:
> https://lore.kernel.org/all/20260616115530.4012675-1-zhoubinbin@loongson.cn/
> 
>  drivers/mfd/ls2k-bmc-core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe..1312812860c0 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -427,14 +427,20 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>   */
>  static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
>  {
> -	char *mode;
> +	char *mode __free(kfree) = NULL;
> +	void __iomem *base;
>  	int depth, ret;
>  
>  	/* The last 16M of PCI BAR0 is used to store the resolution string. */
> -	mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> +	base = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> +	if (!base)
> +		return -ENOMEM;
> +
> +	mode = kmalloc(SZ_16M, GFP_KERNEL);

[Severity: High]
Will this 16MB allocation unconditionally fail on systems where
KMALLOC_MAX_SIZE is smaller, such as 4MB with 4K pages?

Even if successful, is it necessary to synchronously copy 16MB of device
I/O memory just to read a short resolution string?


>  	if (!mode)
>  		return -ENOMEM;
>  
> +	memcpy_fromio(mode, base, SZ_16M);

[Severity: High]
This is a pre-existing issue, but since the buffer is now allocated on the
heap via kmalloc() and copied directly from device memory, will string
functions like strncmp() and strsep() read out-of-bounds if the 16MB
region lacks a null terminator byte?


>  	/* The resolution field starts with the flag "video=". */
>  	if (!strncmp(mode, "video=", 6))
>  		mode = mode + 6;

[Severity: High]
Since mode was declared with __free(kfree), will advancing the pointer
here cause kfree() to be called with a mid-buffer pointer and crash the
allocator when ls2k_bmc_parse_mode() returns?


> 
> base-commit: c454531af72e0df811600601413bb8d3d039ed08
> -- 
> 2.52.0
> 

-- 
Lee Jones

Re: [PATCH v2] mfd: ls2kbmc: mfd: ls2kbmc: Fix iomem pointer handling in video mode parsing

[Binbin Zhou]

Hi Lee:

On Thu, Jul 2, 2026 at 11:57 PM Lee Jones <lee@kernel.org> wrote:
>
> Please consider these:
>
> /* Sashiko Automation: Issues Found (4 Findings) */
>
> On Wed, 24 Jun 2026, Binbin Zhou wrote:
>
> > Use pointers annotated with the __iomem marker for all iomem map calls,
> > and creates a local copy of the mapped IO memory for future access in
> > the code. memcpy_fromio() and memcpy_toio() are used to read/write data
> > from/to mapped IO memory
> >
> > Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202603021730.Yy3QXYTw-lkp@intel.com/
> > Closes: https://lore.kernel.org/oe-kbuild-all/202606120639.WG6eb8VU-lkp@intel.com/
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> > V2:
> >  - Add the missing memcpy_fromio();
> >  - Drop the unnecessary `buf` variable.
> >
> > Link to V1:
> > https://lore.kernel.org/all/20260616115530.4012675-1-zhoubinbin@loongson.cn/
> >
> >  drivers/mfd/ls2k-bmc-core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> > index 408056bfb2fe..1312812860c0 100644
> > --- a/drivers/mfd/ls2k-bmc-core.c
> > +++ b/drivers/mfd/ls2k-bmc-core.c
> > @@ -427,14 +427,20 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
> >   */
> >  static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
> >  {
> > -     char *mode;
> > +     char *mode __free(kfree) = NULL;
> > +     void __iomem *base;
> >       int depth, ret;
> >
> >       /* The last 16M of PCI BAR0 is used to store the resolution string. */
> > -     mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> > +     base = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> > +     if (!base)
> > +             return -ENOMEM;
> > +
> > +     mode = kmalloc(SZ_16M, GFP_KERNEL);
>
> [Severity: High]
> Will this 16MB allocation unconditionally fail on systems where
> KMALLOC_MAX_SIZE is smaller, such as 4MB with 4K pages?
>
> Even if successful, is it necessary to synchronously copy 16MB of device
> I/O memory just to read a short resolution string?

Emm,  indeed, `SZ_64` should be enough. I will also define the following macro:

/* The LS2K BMC display resolution string length store in PCI BAR0 */
#define LS2K_RESOLUTION_STR_LEN                SZ_64

>
>
> >       if (!mode)
> >               return -ENOMEM;
> >
> > +     memcpy_fromio(mode, base, SZ_16M);
>
> [Severity: High]
> This is a pre-existing issue, but since the buffer is now allocated on the
> heap via kmalloc() and copied directly from device memory, will string
> functions like strncmp() and strsep() read out-of-bounds if the 16MB
> region lacks a null terminator byte?
>
>
> >       /* The resolution field starts with the flag "video=". */
> >       if (!strncmp(mode, "video=", 6))
> >               mode = mode + 6;
>
> [Severity: High]
> Since mode was declared with __free(kfree), will advancing the pointer
> here cause kfree() to be called with a mid-buffer pointer and crash the
> allocator when ls2k_bmc_parse_mode() returns?

Yes, I'll add a new variable `pos` and check the return value of
`strncmp()`, as follows:

        /* The resolution field starts with the flag "video=". */
        if (strncmp(mode, "video=", 6)) {
                dev_err(&pdev->dev, "Simpledrm resolution missing or
corrupt!\n");
                return -EINVAL;
        }

        pos = mode + 6;
        ret = kstrtoint(strsep(&pos, "x"), 10, &pd->width);
........
>
>
> >
> > base-commit: c454531af72e0df811600601413bb8d3d039ed08
> > --
> > 2.52.0
> >
>
> --
> Lee Jones

--
Thanks.
Binbin