[PATCH] sysfs: sysfs_pathname/sysfs_add_one: Use strlcat() instead of strcat()

[PATCH] sysfs: sysfs_pathname/sysfs_add_one: Use strlcat() instead of strcat()

[Geert Uytterhoeven]

The warning check for duplicate sysfs entries can cause a buffer overflow
when printing the warning, as strcat() doesn't check buffer sizes.
Use strlcat() instead.

Since strlcat() doesn't return a pointer to the passed buffer, unlike
strcat(), I had to convert the nested concatenation in sysfs_add_one() to
an admittedly more obscure comma operator construct, to avoid emitting code
for the concatenation if CONFIG_BUG is disabled.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: stable@vger.kernel.org
---
 fs/sysfs/dir.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 6b0bb00..2fbdff6 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -485,20 +485,18 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 /**
  *	sysfs_pathname - return full path to sysfs dirent
  *	@sd: sysfs_dirent whose path we want
- *	@path: caller allocated buffer
+ *	@path: caller allocated buffer of size PATH_MAX
  *
  *	Gives the name "/" to the sysfs_root entry; any path returned
  *	is relative to wherever sysfs is mounted.
- *
- *	XXX: does no error checking on @path size
  */
 static char *sysfs_pathname(struct sysfs_dirent *sd, char *path)
 {
 	if (sd->s_parent) {
 		sysfs_pathname(sd->s_parent, path);
-		strcat(path, "/");
+		strlcat(path, "/", PATH_MAX);
 	}
-	strcat(path, sd->s_name);
+	strlcat(path, sd->s_name, PATH_MAX);
 	return path;
 }
 
@@ -531,9 +529,11 @@ int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 		char *path = kzalloc(PATH_MAX, GFP_KERNEL);
 		WARN(1, KERN_WARNING
 		     "sysfs: cannot create duplicate filename '%s'\n",
-		     (path == NULL) ? sd->s_name :
-		     strcat(strcat(sysfs_pathname(acxt->parent_sd, path), "/"),
-		            sd->s_name));
+		     (path == NULL) ? sd->s_name
+				    : (sysfs_pathname(acxt->parent_sd, path),
+				       strlcat(path, "/", PATH_MAX),
+				       strlcat(path, sd->s_name, PATH_MAX),
+				       path));
 		kfree(path);
 	}
 
-- 
1.7.0.4

Re: [PATCH] sysfs: sysfs_pathname/sysfs_add_one: Use strlcat() instead of strcat()

[Greg Kroah-Hartman]

On Sat, Sep 29, 2012 at 10:23:19PM +0200, Geert Uytterhoeven wrote:
> The warning check for duplicate sysfs entries can cause a buffer overflow
> when printing the warning, as strcat() doesn't check buffer sizes.
> Use strlcat() instead.

As the comment said, we knew about this, but I have never seen it, do
you know of a way to trigger it?

> Since strlcat() doesn't return a pointer to the passed buffer, unlike
> strcat(), I had to convert the nested concatenation in sysfs_add_one() to
> an admittedly more obscure comma operator construct, to avoid emitting code
> for the concatenation if CONFIG_BUG is disabled.

That's a fun construct, nice work.

> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: stable@vger.kernel.org

Given that I don't know of any way to actually hit this problem, is it
really needed for older kernel releases?

thanks,

greg k-h

Re: [PATCH] sysfs: sysfs_pathname/sysfs_add_one: Use strlcat() instead of strcat()

[Geert Uytterhoeven]

Hi Greg,

On Sun, Sep 30, 2012 at 4:49 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 29, 2012 at 10:23:19PM +0200, Geert Uytterhoeven wrote:
>> The warning check for duplicate sysfs entries can cause a buffer overflow
>> when printing the warning, as strcat() doesn't check buffer sizes.
>> Use strlcat() instead.
>
> As the comment said, we knew about this, but I have never seen it, do
> you know of a way to trigger it?

I expected there would be a check somewhere else in the code, so we can
never overflow here. But I did manage to overflow the buffer by having a real
long name (4060 characters) in a conflicting mfd_cell.
There may be other ways.

I don't know how likely it is to trigger in a real world scenario. Is
there a limit
on the depth of sysfs? Or can it go unbounded, e.g. by cascading USB hubs?

>> Cc: stable@vger.kernel.org
>
> Given that I don't know of any way to actually hit this problem, is it
> really needed for older kernel releases?

That's up to you to decide...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sysfs: sysfs_pathname/sysfs_add_one: Use strlcat() instead of strcat()

[Greg Kroah-Hartman]

On Sun, Sep 30, 2012 at 08:59:05PM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Sun, Sep 30, 2012 at 4:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Sep 29, 2012 at 10:23:19PM +0200, Geert Uytterhoeven wrote:
> >> The warning check for duplicate sysfs entries can cause a buffer overflow
> >> when printing the warning, as strcat() doesn't check buffer sizes.
> >> Use strlcat() instead.
> >
> > As the comment said, we knew about this, but I have never seen it, do
> > you know of a way to trigger it?
> 
> I expected there would be a check somewhere else in the code, so we can
> never overflow here. But I did manage to overflow the buffer by having a real
> long name (4060 characters) in a conflicting mfd_cell.
> There may be other ways.
> 
> I don't know how likely it is to trigger in a real world scenario. Is
> there a limit
> on the depth of sysfs? Or can it go unbounded, e.g. by cascading USB hubs?

You can only go so deep with USB hubs (5 I think.)

> >> Cc: stable@vger.kernel.org
> >
> > Given that I don't know of any way to actually hit this problem, is it
> > really needed for older kernel releases?
> 
> That's up to you to decide...

Ok, I'll queue this up after 3.7-rc1 is out, thanks.

greg k-h