| Message ID | 20210423080805.16535-1-goto@k-tech.co.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] IOMUX: Fix buffer overflow in iomux_replace_device() | expand |
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?
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.
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;
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(-)