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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Chris Wedgwood]

+ /*
+ * 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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[YOSHIFUJI Hideaki / 吉藤英明]

Post by Tetsuo Handa
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 <***@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[YOSHIFUJI Hideaki / 吉藤英明]

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

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 <***@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 <***@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Tetsuo Handa]

Hi,

I understood it will not cause trouble.

Thanks a lot.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[YOSHIFUJI Hideaki / 吉藤英明]

Post by YOSHIFUJI Hideaki / 吉藤英明
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.)

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

Signed-off-by: Hideaki YOSHIFUJI <***@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 <***@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[YOSHIFUJI Hideaki / 吉藤英明]

+ * 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Willy Tarreau]

Hi,

Post by 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;

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------