[PATCH v2] IOMUX: Fix buffer overflow in iomux_replace_device()

[PATCH v2] IOMUX: Fix buffer overflow in iomux_replace_device()

[Yuichiro Goto]

Use of strcat() against an uninitialized buffer would lead
to buffer overflow. This patch fixes it.

Signed-off-by: Yuichiro Goto <goto@k-tech.co.jp>
Cc: Peter Robinson <pbrobinson@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---

Changes for v2:
- Add "IOMUX" in title

 common/iomux.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/iomux.c b/common/iomux.c
index b9088aa3b5..27baca6931 100644
--- a/common/iomux.c
+++ b/common/iomux.c
@@ -158,7 +158,10 @@ int iomux_replace_device(const int console, const char *old, const char *new)
 			return -ENOMEM;
 		}
 
-		strcat(tmp, ",");
+		if (!arg)
+			strcpy(tmp, ",");
+		else
+			strcat(tmp, ",");
 		strcat(tmp, name);
 
 		arg = tmp;
-- 
2.17.1

[PATCH v2] IOMUX: Fix buffer overflow in iomux_replace_device()

[Andy Shevchenko]

On Fri, Apr 23, 2021 at 05:08:05PM +0900, Yuichiro Goto wrote:
> Use of strcat() against an uninitialized buffer would lead
> to buffer overflow. This patch fixes it.

Thanks for report!

Can you also add a Fixes tag?

> Signed-off-by: Yuichiro Goto <goto@k-tech.co.jp>
> Cc: Peter Robinson <pbrobinson@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
> 
> Changes for v2:
> - Add "IOMUX" in title
> 
>  common/iomux.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/common/iomux.c b/common/iomux.c
> index b9088aa3b5..27baca6931 100644
> --- a/common/iomux.c
> +++ b/common/iomux.c
> @@ -158,7 +158,10 @@ int iomux_replace_device(const int console, const char *old, const char *new)
>  			return -ENOMEM;
>  		}
>  
> -		strcat(tmp, ",");
> +		if (!arg)
> +			strcpy(tmp, ",");
> +		else
> +			strcat(tmp, ",");
>  		strcat(tmp, name);

It would be better to have it like

	if (arg) {
		strcat(tmp, ",");
		strcat(tmp, name);
	} else
		strcpy(tmp, name);


>  		arg = tmp;

Peter, does it fix your issue seen on ARMv7?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] IOMUX: Fix buffer overflow in iomux_replace_device()

[Yuichiro Goto]

On Fri, Apr 23, 2021 at 10:09 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Apr 23, 2021 at 05:08:05PM +0900, Yuichiro Goto wrote:
> > Use of strcat() against an uninitialized buffer would lead
> > to buffer overflow. This patch fixes it.
>
> Thanks for report!
>
> Can you also add a Fixes tag?
>
> > Signed-off-by: Yuichiro Goto <goto@k-tech.co.jp>
> > Cc: Peter Robinson <pbrobinson@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >
> > Changes for v2:
> > - Add "IOMUX" in title
> >
> >  common/iomux.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/iomux.c b/common/iomux.c
> > index b9088aa3b5..27baca6931 100644
> > --- a/common/iomux.c
> > +++ b/common/iomux.c
> > @@ -158,7 +158,10 @@ int iomux_replace_device(const int console, const char *old, const char *new)
> >                       return -ENOMEM;
> >               }
> >
> > -             strcat(tmp, ",");
> > +             if (!arg)
> > +                     strcpy(tmp, ",");
> > +             else
> > +                     strcat(tmp, ",");
> >               strcat(tmp, name);
>
> It would be better to have it like
>
>         if (arg) {
>                 strcat(tmp, ",");
>                 strcat(tmp, name);
>         } else
>                 strcpy(tmp, name);
>
>
> >               arg = tmp;
>
> Peter, does it fix your issue seen on ARMv7?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Hi Andy,

Thank you for your review. I submitted the patch v3 based on
you suggestions.

-- 
Kind Regards,
Yuichiro Goto