Off-by-one bug at unix_mkname ?

Off-by-one bug at unix_mkname ?

[Tetsuo Handa]

Hi,

It seems to me that the following code is off-by-one bug.

http://lxr.linux.no/source/net/unix/af_unix.c#L191
http://lxr.linux.no/source/net/unix/af_unix.c?v=2.4.28#L182

I think
((char *)sunaddr)[len]=0;
should be
((char *)sunaddr)[len-1]=0;


Thanks.

Re: Off-by-one bug at unix_mkname ?

[Willy Tarreau]

Hi,

On Mon, Mar 28, 2005 at 05:00:05PM +0900, Tetsuo Handa wrote:
> Hi,
> 
> It seems to me that the following code is off-by-one bug.
> 
> http://lxr.linux.no/source/net/unix/af_unix.c#L191
> http://lxr.linux.no/source/net/unix/af_unix.c?v=2.4.28#L182
> 
> I think
> ((char *)sunaddr)[len]=0;
> should be
> ((char *)sunaddr)[len-1]=0;

it seems you're right, or the first test in the function is wrong, so
there's clearly something to be fixed there :

static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
{
	if (len <= sizeof(short) || len > sizeof(*sunaddr))
                                    ^^^^^^^^^^^^^^^^^^^^^^
		return -EINVAL;
	if (!sunaddr || sunaddr->sun_family != AF_UNIX)
		return -EINVAL;
	if (sunaddr->sun_path[0]) {
		((char *)sunaddr)[len]=0;
                        ^^^^^^^^^^^^^^
		len = strlen(sunaddr->sun_path)+1+sizeof(short);
		return len;
	}

	*hashp = unix_hash_fold(csum_partial((char*)sunaddr, len, 0));
	return len;
}


Then, I would propose this patch (both for 2.4 and 2.6) :

--- ./net/unix/af_unix.c.bad	Sat Mar 26 07:42:49 2005
+++ ./net/unix/af_unix.c	Mon Mar 28 10:11:25 2005
@@ -179,7 +179,7 @@
 	if (!sunaddr || sunaddr->sun_family != AF_UNIX)
 		return -EINVAL;
 	if (sunaddr->sun_path[0]) {
-		((char *)sunaddr)[len]=0;
+		((char *)sunaddr)[len-1]=0;
 		len = strlen(sunaddr->sun_path)+1+sizeof(short);
 		return len;
 	}

Regards,
Willy

Re: Off-by-one bug at unix_mkname ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <200503281700.HHE91205.FtVLOStGOSPMYJFMN@I-love.sakura.ne.jp> (at Mon, 28 Mar 2005 17:00:05 +0900), Tetsuo Handa <from-linux-kernel@I-love.sakura.ne.jp> says:

> It seems to me that the following code is off-by-one bug.
> 
> http://lxr.linux.no/source/net/unix/af_unix.c#L191
> http://lxr.linux.no/source/net/unix/af_unix.c?v=2.4.28#L182
> 
> I think
> ((char *)sunaddr)[len]=0;
> should be
> ((char *)sunaddr)[len-1]=0;

Well, 2.2 has some comment on this:

static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
{
        if (len <= sizeof(short) || len > sizeof(*sunaddr))
                return -EINVAL;
:
        if (sunaddr->sun_path[0])
        {
                /*
                 *      This may look like an off by one error but it is
                 *      a bit more subtle. 108 is the longest valid AF_UNIX
                 *      path for a binding. sun_path[108] doesnt as such
                 *      exist. However in kernel space we are guaranteed that
                 *      it is a valid memory location in our kernel
                 *      address buffer.
                 */
                if (len > sizeof(*sunaddr))
                        len = sizeof(*sunaddr);
                ((char *)sunaddr)[len]=0;
                len = strlen(sunaddr->sun_path)+1+sizeof(short);
                return len;
        }
:

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Off-by-one bug at unix_mkname ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050328.172108.30349253.yoshfuji@linux-ipv6.org> (at Mon, 28 Mar 2005 17:21:08 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> > It seems to me that the following code is off-by-one bug.
:
> Well, 2.2 has some comment on this:

So, I'd suggest to put the comment back to 2.4/2.6 instead.
(Note: net/socket.c refers this around MAX_SOCK_ADDR definition.)

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== net/unix/af_unix.c 1.73 vs edited =====
--- 1.73/net/unix/af_unix.c	2005-03-10 13:42:53 +09:00
+++ edited/net/unix/af_unix.c	2005-03-28 17:31:33 +09:00
@@ -188,6 +188,15 @@
 	if (!sunaddr || sunaddr->sun_family != AF_UNIX)
 		return -EINVAL;
 	if (sunaddr->sun_path[0]) {
+		/*
+		 *	This may look like an off by one error but it is
+		 *	a bit more subtle. 108 is the longest valid AF_UNIX
+		 *	path for a binding. sun_path[108] doesnt as such
+		 *	exist. However in kernel space we are guaranteed that
+		 *	it is a valid memory location in our kernel
+		 *	address buffer.
+		 */
+		if (len > sizeof(*sunaddr))
 		((char *)sunaddr)[len]=0;
 		len = strlen(sunaddr->sun_path)+1+sizeof(short);
 		return len;

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Off-by-one bug at unix_mkname ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050328.173938.26746686.yoshfuji@linux-ipv6.org> (at Mon, 28 Mar 2005 17:39:38 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> So, I'd suggest to put the comment back to 2.4/2.6 instead.
> (Note: net/socket.c refers this around MAX_SOCK_ADDR definition.)
> 
> Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

Oops, sorry, I made a mistake when I did copy-n-paste...

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== net/unix/af_unix.c 1.73 vs edited =====
--- 1.73/net/unix/af_unix.c	2005-03-10 13:42:53 +09:00
+++ edited/net/unix/af_unix.c	2005-03-28 17:45:26 +09:00
@@ -188,6 +188,14 @@
 	if (!sunaddr || sunaddr->sun_family != AF_UNIX)
 		return -EINVAL;
 	if (sunaddr->sun_path[0]) {
+		/*
+		 *	This may look like an off by one error but it is
+		 *	a bit more subtle. 108 is the longest valid AF_UNIX
+		 *	path for a binding. sun_path[108] doesnt as such
+		 *	exist. However in kernel space we are guaranteed that
+		 *	it is a valid memory location in our kernel
+		 *	address buffer.
+		 */
 		((char *)sunaddr)[len]=0;
 		len = strlen(sunaddr->sun_path)+1+sizeof(short);
 		return len;

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Off-by-one bug at unix_mkname ?

[Chris Wedgwood]

On Mon, Mar 28, 2005 at 05:39:38PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:

> +		/*
> +		 *	This may look like an off by one error but it is
> +		 *	a bit more subtle. 108 is the longest valid AF_UNIX
> +		 *	path for a binding. sun_path[108] doesnt as such
> +		 *	exist. However in kernel space we are guaranteed that
> +		 *	it is a valid memory location in our kernel
> +		 *	address buffer.

icky pointless white space?

> +		 */
> +		if (len > sizeof(*sunaddr))

what?

Re: Off-by-one bug at unix_mkname ?

[Tetsuo Handa]

Hi,

I understood it will not cause trouble.

Thanks a lot.

Re: Off-by-one bug at unix_mkname ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <Pine.LNX.4.61.0503281124450.18443@yvahk01.tjqt.qr> (at Mon, 28 Mar 2005 11:25:39 +0200 (MEST)), Jan Engelhardt <jengelh@linux01.gwdg.de> says:

> 
> On Mar 28 2005 17:39, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> 
> >+		 *	This may look like an off by one error but it is
> >+		 *	a bit more subtle. 108 is the longest valid AF_UNIX
> >+		 *	path for a binding. sun_path[108] doesnt as such
> >+		 *	exist. However in kernel space we are guaranteed that
> >+		 *	it is a valid memory location in our kernel
> >+		 *	address buffer.
> >+		 */
> 
> Now, does 2.6. _still_ guarantee that 108 is a valid offset?

Yes, it does.

--yoshfuji