[patch] Fix handling of overlength pathname in AF_UNIX sun_path

[patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Michael Kerrisk]

Tetsuo Handa sent me a patch to document the kernel's odd 
behavior when asked to create a UNIX domain socket address where the 
pathname completely fills the sun_path field without including a null 
terminator [1]. 

One of the consequences of the current kernel behavior is that 
when a socket address is returned to userspace (via getsockname(), 
getpeername(), accept(), recvfrom()), applications can't reliably 
do things such as:

printf("%s\n", addr.sun_path);

Instead one must either write:

printf("%.*s\n", sizeof(addr.sun_path), addr.sun_path);

or, when retrieving a socket address structure, employ a buffer whose
size is:

sizeof(struct sockaddr_un) + 1

(This ensures that there is enough space to hold the null terminator
for the case where a socket was bound to a sun_path with non-NUL characters
in all 108 bytes. But it entails some casting.)

Tetsuo initially considered there might be a kernel bug here, 
but an attempt to change the kernel behavior met resistance [2].

The patch at the end of this message is a slightly different 
fix for the same problem. There are a few reasons why I think 
it (or some variation) should be considered:

1. Changing the kernel behavior prevents userspace having 
   to go through the sort of contortions described above
   in order to handle the 108-non-NUL-bytes-in-sun_path case.
2. POSIX says that sun_path is a pathname. By (POSIX) definition, 
   a pathname is null terminated.
3. Considering these two sets:

   (a) [applications that rely on the assumption that there
        is a null terminator inside sizeof(sun_path) bytes]
   (b) [applications that would break if the kernel behavior changed]

   I suspect that set (a) is rather larger than set (b)--or, more 
   likely still, applications ensure they go for the lowest common
   denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
   bytes, and so avoid this issue completely.

The accompanying patch changes unix_mkname() to ensure that a terminating 
null byte is always located within the first 108 bytes of sun_path. 
It does change the ABI for the former case where a pathname ran to 108
bytes without a null terminator: for that case, the call now fails with
the error -EINVAL. What are people's thoughts on applying this?

[1] http://thread.gmane.org/gmane.linux.network/174473/focus=1861
[2] http://thread.gmane.org/gmane.linux.kernel/291038

Signed-off-by: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

---

--- net/unix/af_unix.c.orig	2012-04-17 09:50:07.383459311 +1200
+++ net/unix/af_unix.c	2012-04-17 19:49:56.077852639 +1200
@@ -207,14 +207,16 @@ static int unix_mkname(struct sockaddr_u
 	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] doesn't 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;
+		if (len == sizeof(*sunaddr)) {
+			/*
+			 * If 'sun_path' is completely filled, the user
+			 * must include a terminator
+			 */
+			if (!memchr(sunaddr->sun_path, '\0',
+					sizeof(sunaddr->sun_path)))
+				return -EINVAL;
+		} else
+			((char *)sunaddr)[len] = 0;
 		len = strlen(sunaddr->sun_path)+1+sizeof(short);
 		return len;
 	}

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Willy Tarreau]

Hi Michael,

On Tue, Apr 17, 2012 at 10:44:15PM +1200, Michael Kerrisk wrote:
(...)
> The accompanying patch changes unix_mkname() to ensure that a terminating 
> null byte is always located within the first 108 bytes of sun_path. 
> It does change the ABI for the former case where a pathname ran to 108
> bytes without a null terminator: for that case, the call now fails with
> the error -EINVAL. What are people's thoughts on applying this?

My personal opinion is that (as you said), the risk of breaking existing
apps is already fairly low, but we must not deliberately break existing
apps. Eventhough there are currently a log, this is exactly what sysctls
are made for. I would personally like to have a default limit to 107 chars 
+ one zero, with a sysctl option to revert to current behaviour if ever it
broke an application. In my opinion it's exactly comparable to the risk of
breaking apps with mmap_min_addr : very low risk but must be covered by a
workaround (sysctl).

Just my 2 cents,
Willy

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Carlos O'Donell]

On Tue, Apr 17, 2012 at 6:51 AM, Willy Tarreau <w@1wt.eu> wrote:
> Hi Michael,
>
> On Tue, Apr 17, 2012 at 10:44:15PM +1200, Michael Kerrisk wrote:
> (...)
>> The accompanying patch changes unix_mkname() to ensure that a terminating
>> null byte is always located within the first 108 bytes of sun_path.
>> It does change the ABI for the former case where a pathname ran to 108
>> bytes without a null terminator: for that case, the call now fails with
>> the error -EINVAL. What are people's thoughts on applying this?
>
> My personal opinion is that (as you said), the risk of breaking existing
> apps is already fairly low, but we must not deliberately break existing
> apps. Eventhough there are currently a log, this is exactly what sysctls
> are made for. I would personally like to have a default limit to 107 chars
> + one zero, with a sysctl option to revert to current behaviour if ever it
> broke an application. In my opinion it's exactly comparable to the risk of
> breaking apps with mmap_min_addr : very low risk but must be covered by a
> workaround (sysctl).

To further the opinion that the risk is low:

The Open Solaris SUN_LEN macro uses strlen to compute the length.

The glibc SUN_LEN macro uses strlen to compute the length.

The Mac OS X libc SUN_LEN macro uses strlen to compute the length.

The linux man-pages project sets the user expectation that it is a
`null-terminated string' (circular argument).

I see every expectation from userspace/glibc that it should be 107 chars + '\0'.

Cheers,
Carlos.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Alan Cox]

I think its pointless.

Yes there may have been a case years ago to nul terminate but thats now
how 4BSD defined the API so that's how the world looks.

It helps nobody on the app side because they'll be defensively coding for
the existing API for another ten years for enterprise distros anyway.

Alan

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Miller]

From: Michael Kerrisk <mtk.manpages@gmail.com>
Date: Tue, 17 Apr 2012 22:44:15 +1200

> 1. Changing the kernel behavior prevents userspace having 
>    to go through the sort of contortions described above
>    in order to handle the 108-non-NUL-bytes-in-sun_path case.

The problem with this logic is that it ignores every single Linux
system that exists right now.

You need to code this logic into your application unless you don't
want it to run properly on every Linux system that actually exists.

Sorry, we're not making this change.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Carlos O'Donell]

On Tue, Apr 17, 2012 at 10:36 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Tue, 17 Apr 2012 22:44:15 +1200
>
>> 1. Changing the kernel behavior prevents userspace having
>>    to go through the sort of contortions described above
>>    in order to handle the 108-non-NUL-bytes-in-sun_path case.
>
> The problem with this logic is that it ignores every single Linux
> system that exists right now.
>
> You need to code this logic into your application unless you don't
> want it to run properly on every Linux system that actually exists.
>
> Sorry, we're not making this change.

Dave,

I don't clearly understand your position here, and perhaps that's my
own ignorance, but could you please clarify, with examples, exactly
why the change is not acceptable? I can see several valid arguments
against the change, but I don't know which argument your position
asserts.

One might assert that careless userspace applications exist that pass
`sizeof(struct sockaddr_un)' (or worse) as the 3rd argument to bind
instead of SUN_LEN(my_sock). The logic in the patch doesn't account
for this, and can't really, and would therefore unacceptably break
existing applications by trying to assert the location of a \0 where
one doesn't exist. The kernel must therefore continue to
null-terminate at the specified length, possibly the 109th character,
and use strlen to capture the true length of the path. The kernel
knows that in the worst case a non-null terminated path might contain
some garbage, but that's the users fault, and the logic to prevent
this must exist in the application not the kernel.

The counter argument to all of this is that it's a QoI issue, and that
the kernel shouldn't accept accidentally non-null terminated paths,
and should instead return EINVAL for them. Not to mention that it's
difficult for userspace to easily catch this error in glibc which
would need to inspect the sockaddr, duplicating kernel code.

Why is it valid for the user path to have no null terminator?

Why not have:

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index d510353..f9f77a7 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un
*sunaddr, int len, unsigned *hashp)
                 */
                ((char *)sunaddr)[len] = 0;
                len = strlen(sunaddr->sun_path)+1+sizeof(short);
+               /* No null terminator was found in the path. */
+               if (len > sizeof(*sunaddr))
+                       return -EINVAL;
                return len;
        }

---

Does that make sense?

Cheers,
Carlos.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Miller]

From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org>
Date: Wed, 18 Apr 2012 00:08:47 -0400

> I don't clearly understand your position here, and perhaps that's my
> own ignorance, but could you please clarify, with examples, exactly
> why the change is not acceptable?

My position is that since millions upon millions of Linux systems, in
fact every single Linux system, exists right now with the current
behavior we are not helping application writers at all by changing
behavior now after it's been this way for nearly 20 years.

Because if an application writer wants his code to work on systems
that actually exist he has to accomodate the non-NULL termination
situation if he wants to inspect or print out an AF_UNIX path.

Because every system in existence right now allows the non-NULL
terminated AF_UNIX paths, therefore it's possible on every system
in existence right now.

Catch my drift?

The very thing the patch claims to help, it doesn't.  We install this
kernel patch now and then tell application writers that they can just
assume all AF_UNIX paths are NULL terminated when they want to print
it out, because such code will not actually be guarenteed to work on
all deployed Linux machines out there.

You cannot just ignore 20 years of precedence and say "oh let's change
this in the kernel now, and that way application writers don't have to
worry about that lack of NULL termination any more."  It simply
doesn't work like that.

All of this talk about whether applications actually create non-NULL
terminated AF_UNIX paths don't even factor into the conversation.

So the value proposition for this patch simply does not exist.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Carlos O'Donell]

On Wed, Apr 18, 2012 at 12:16 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org>
> Date: Wed, 18 Apr 2012 00:08:47 -0400
>
>> I don't clearly understand your position here, and perhaps that's my
>> own ignorance, but could you please clarify, with examples, exactly
>> why the change is not acceptable?
>
> My position is that since millions upon millions of Linux systems, in
> fact every single Linux system, exists right now with the current
> behavior we are not helping application writers at all by changing
> behavior now after it's been this way for nearly 20 years.
>
> Because if an application writer wants his code to work on systems
> that actually exist he has to accomodate the non-NULL termination
> situation if he wants to inspect or print out an AF_UNIX path.
>
> Because every system in existence right now allows the non-NULL
> terminated AF_UNIX paths, therefore it's possible on every system
> in existence right now.
>
> Catch my drift?
>
> The very thing the patch claims to help, it doesn't.  We install this
> kernel patch now and then tell application writers that they can just
> assume all AF_UNIX paths are NULL terminated when they want to print
> it out, because such code will not actually be guarenteed to work on
> all deployed Linux machines out there.
>
> You cannot just ignore 20 years of precedence and say "oh let's change
> this in the kernel now, and that way application writers don't have to
> worry about that lack of NULL termination any more."  It simply
> doesn't work like that.
>
> All of this talk about whether applications actually create non-NULL
> terminated AF_UNIX paths don't even factor into the conversation.
>
> So the value proposition for this patch simply does not exist.

Thank you, this is the kind of position statement I can point to if I
ever get asked about this again.

In summary your opinion is that the API has and always will allow up
to 108 chars to be used in sun_path?

In which case I will talk to the Austin group to get a good example
added to POSIX showing safe usage.

Cheers,
Carlos.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Miller]

From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org>
Date: Wed, 18 Apr 2012 08:57:58 -0400

> In summary your opinion is that the API has and always will allow up
> to 108 chars to be used in sun_path?

Yes.

> In which case I will talk to the Austin group to get a good example
> added to POSIX showing safe usage.

Why would you add language to POSIX for Linux specific behavior?
Just curious :-)

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Carlos O'Donell]

On Wed, Apr 18, 2012 at 1:31 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org>
> Date: Wed, 18 Apr 2012 08:57:58 -0400
>
>> In summary your opinion is that the API has and always will allow up
>> to 108 chars to be used in sun_path?
>
> Yes.
>
>> In which case I will talk to the Austin group to get a good example
>> added to POSIX showing safe usage.
>
> Why would you add language to POSIX for Linux specific behavior?
> Just curious :-)

Why not? Do you ever feel crazy when people question what you think is
perfectly reasonable? ;-)

POSIX doesn't exist in a vacuum, we need to harmonize reality with the
standard. If an implementation exists where sun_path has no
null-terminator then it is useful to have POSIX clarify that
null-termination is implementation defined behaviour, just like it
says that sun_path's length undefined. Under "Application Usage" or
"Examples" it's valid to talk about specific implementations.

See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html,
where it talks about BSD in the "Application Usage." It's about time
we some "Linux this" and "Linux that" in there.

Cheers,
Carlos.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Miller]

From: "Carlos O'Donell" <carlos@systemhalted.org>
Date: Wed, 18 Apr 2012 14:48:43 -0400

> POSIX doesn't exist in a vacuum, we need to harmonize reality with the
> standard. If an implementation exists where sun_path has no
> null-terminator then it is useful to have POSIX clarify that
> null-termination is implementation defined behaviour, just like it
> says that sun_path's length undefined. Under "Application Usage" or
> "Examples" it's valid to talk about specific implementations.
> 
> See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html,
> where it talks about BSD in the "Application Usage." It's about time
> we some "Linux this" and "Linux that" in there.

Ok, thanks for explaining.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Michael Kerrisk (man-pages)]

On Wed, Apr 18, 2012 at 4:16 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org>
> Date: Wed, 18 Apr 2012 00:08:47 -0400
>
>> I don't clearly understand your position here, and perhaps that's my
>> own ignorance, but could you please clarify, with examples, exactly
>> why the change is not acceptable?
>
> My position is that since millions upon millions of Linux systems, in
> fact every single Linux system, exists right now with the current
> behavior we are not helping application writers at all by changing
> behavior now after it's been this way for nearly 20 years.
>
> Because if an application writer wants his code to work on systems
> that actually exist he has to accomodate the non-NULL termination
> situation if he wants to inspect or print out an AF_UNIX path.
>
> Because every system in existence right now allows the non-NULL
> terminated AF_UNIX paths, therefore it's possible on every system
> in existence right now.
>
> Catch my drift?
>
> The very thing the patch claims to help, it doesn't.  We install this
> kernel patch now and then tell application writers that they can just
> assume all AF_UNIX paths are NULL terminated when they want to print
> it out, because such code will not actually be guarenteed to work on
> all deployed Linux machines out there.

Hang on a moment. I did not suggest that we can just tell users they
can forget about the past. Obviously, users will need to program to
past kernel behavior here for a good long time yet. (As Alan says
elsewhere in the thread "they'll be defensively coding for
the existing API for another ten years for enterprise distros
anyway".) However, this is about longer-term improvement of the
quality of implementation; in X years (choose your X) time, a lot of
new application may not need to care about the old broken behavior.
See some related examples below.

And you skipped past my other two points. Even if my understanding
about POSIX mandates is correct, I can understand how we might ignore
that point. But the last one is still germane:

[[
3. Considering these two sets:

  (a) [applications that rely on the assumption that there
       is a null terminator inside sizeof(sun_path) bytes]
  (b) [applications that would break if the kernel behavior changed]

  I suspect that set (a) is rather larger than set (b)--or, more
  likely still, applications ensure they go for the lowest common
  denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
  bytes, and so avoid this issue completely.
]]

There may well be potential breakages out there in set (a), and
improving the QOI would help them. (To put things in terms of Alan's
response: I suspect that there may well be existing applications that
are *not* defensively handling the existing API).

Taking the logic you've posed (my reading: "we shouldn't fix old
brokenness because applications will still need to code to the
brokenness") to the extreme, we'd *never* fix old pieces of
brokenness. However, we certainly have precedents for doing exactly
that:

After nearly 15 years of brokenness (stretching back to the first
kernels), commit 69be8f189653cd81aae5a74e26615b12871bb72e fixed this
(sigaction(2)):

   BUGS
       In kernels up to and including 2.6.13, specifying SA_NODEFER in
       sa_flags  prevents  not  only  the  delivered signal from being
       masked during execution of the handler, but  also  the  signals
       specified in sa_mask.  This bug was fixed in kernel 2.6.14.

Similarly, after brokenness that had run through the entire preceding
2.4.x kernel series, Linux 2.6.4 fixed this:

    BUGS
       In kernel 2.4 (and earlier) there is some  strangeness  in  the
       handling  of  X_OK  tests  for superuser.  If all categories of
       execute permission are disabled for a nondirectory  file,  then
       the  only  access() test that returns -1 is when mode is speci‐
       fied as just X_OK; if R_OK or W_OK is also specified  in  mode,
       then  access() returns 0 for such files.  Early 2.6 kernels (up
       to and including 2.6.3) also behaved in the same way as  kernel
       2.4.

(A little background here:
http://thread.gmane.org/gmane.linux.kernel/158814, and the fix
eventually went in with
http://thread.gmane.org/gmane.linux.kernel/178719)

> You cannot just ignore 20 years of precedence and say "oh let's change
> this in the kernel now, and that way application writers don't have to
> worry about that lack of NULL termination any more."  It simply
> doesn't work like that.

As should be clear from the above, I agree. But still, I don't think
the logic "it's broken, and even if we fix it, users will still have
to code to the old brokenness" is a sufficient argument against
improving the QOI long-term.

> All of this talk about whether applications actually create non-NULL
> terminated AF_UNIX paths don't even factor into the conversation.
>
> So the value proposition for this patch simply does not exist.

Of course, it's your call in the end, but I don't think things are as
cut-and-dried as your response suggests.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Miller]

From: "Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Thu, 19 Apr 2012 10:50:40 +1200

> However, this is about longer-term improvement of the quality of
> implementation; in X years (choose your X) time, a lot of new
> application may not need to care about the old broken behavior.

There is really no value to this, the AF_UNIX NULL termination issue
is significantly different from the signal examples you mention.

If we're going to, like Carlos will, make mention in POSIX documents
that one must account for possible lack of NULL termination, there
is absolutely ZERO value in changing things because we are telling
application writers the state of reality which is that they have
to allot for this.

Please drop this issue, the discussion was over a long time ago, thank
you very much.

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Alan Cox]

> 3. Considering these two sets:
> 
>   (a) [applications that rely on the assumption that there
>        is a null terminator inside sizeof(sun_path) bytes]
>   (b) [applications that would break if the kernel behavior changed]
> 
>   I suspect that set (a) is rather larger than set (b)--or, more
>   likely still, applications ensure they go for the lowest common
>   denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
>   bytes, and so avoid this issue completely.

Or another way of putting it

3(a)	Sloppy coding that may have lots of other bugs

3(b)	Interfaces and code we promised not to break.



Alan

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Michael Kerrisk (man-pages)]

On Thu, Apr 19, 2012 at 10:19 PM, Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:
>> 3. Considering these two sets:
>>
>>   (a) [applications that rely on the assumption that there
>>        is a null terminator inside sizeof(sun_path) bytes]
>>   (b) [applications that would break if the kernel behavior changed]
>>
>>   I suspect that set (a) is rather larger than set (b)--or, more
>>   likely still, applications ensure they go for the lowest common
>>   denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
>>   bytes, and so avoid this issue completely.
>
> Or another way of putting it
>
> 3(a)    Sloppy coding that may have lots of other bugs
>
> 3(b)    Interfaces and code we promised not to break.

Yes, it's another way of putting it. (Though regarding 3(b), part of
the problem is that there never was a clearly specified contract.)

Anyway, I've dug deeper, looking at hat happens on other platforms.
It's a mess: the BSDs don't even guarantee  that sun_path is
null_terminated. So, here's how one has to portably deal with the
variations:

addrlen = sizeof(struct sockaddr_un);
cfd = accept(lfd, &addr, &addlen);

printf("%.*s", addrlen - offsetof(struct sockaddr_un, sun_path), addr.sun_path);

That's pretty hideous!

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Jan Engelhardt]

On Thursday 2012-04-19 12:33, Michael Kerrisk (man-pages) wrote:
>
>Anyway, I've dug deeper, looking at hat happens on other platforms.
>It's a mess: the BSDs don't even guarantee  that sun_path is
>null_terminated. So, here's how one has to portably deal with the
>variations:
>
>addrlen = sizeof(struct sockaddr_un);
>cfd = accept(lfd, &addr, &addlen);
>
>printf("%.*s", addrlen - offsetof(struct sockaddr_un, sun_path), addr.sun_path);

What operating system made you write that?


I just ask for fun and the record. Solaris's dirent is also
constructed such that using sizeof(...) is useless.

typedef struct dirent {
        ino_t           d_ino;          /* "inode number" of entry */
        off_t           d_off;          /* offset of disk directory entry */
        unsigned short  d_reclen;       /* length of this record */
        char            d_name[1];      /* name of file */
} dirent_t;

RE: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Laight]

 
> 
> Why not have:
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index d510353..f9f77a7 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un
> *sunaddr, int len, unsigned *hashp)
>                  */
>                 ((char *)sunaddr)[len] = 0;
>                 len = strlen(sunaddr->sun_path)+1+sizeof(short);
> +               /* No null terminator was found in the path. */
> +               if (len > sizeof(*sunaddr))
> +                       return -EINVAL;
>                 return len;

That could generate a kernel page fault!
(Depending on what follows (or rather doesn't follow!) sun_path.)
You'd need to use memchr() not strlen().

	David

RE: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[David Laight]

 
> 
> Why not have:
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index d510353..f9f77a7 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un
> *sunaddr, int len, unsigned *hashp)
>                  */
>                 ((char *)sunaddr)[len] = 0;
>                 len = strlen(sunaddr->sun_path)+1+sizeof(short);
> +               /* No null terminator was found in the path. */
> +               if (len > sizeof(*sunaddr))
> +                       return -EINVAL;
>                 return len;

That could generate a kernel page fault!
(Depending on what follows (or rather doesn't follow!) sun_path.)
You'd need to use memchr() not strlen().

	David

Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path

[Thadeu Lima de Souza Cascardo]

On Wed, Apr 18, 2012 at 09:17:26AM +0100, David Laight wrote:
> 
> > 
> > Why not have:
> > 
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index d510353..f9f77a7 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un
> > *sunaddr, int len, unsigned *hashp)
> >                  */
> >                 ((char *)sunaddr)[len] = 0;
> >                 len = strlen(sunaddr->sun_path)+1+sizeof(short);
> > +               /* No null terminator was found in the path. */
> > +               if (len > sizeof(*sunaddr))
> > +                       return -EINVAL;
> >                 return len;
> 
> That could generate a kernel page fault!
> (Depending on what follows (or rather doesn't follow!) sun_path.)
> You'd need to use memchr() not strlen().
> 
> 	David
> 

Hi, David.

What follows is a 0 byte, because it's set that way in the line before
strlen. Note that len is tested for sizeof(*sunaddr), and there is a
huge comment about that extra byte that was omitted.

The whole function is at net/unix/af_unix.c:203.

Regards,
Cascardo.