[PATCH][UML] Boot broken due to buffer overrun

[PATCH][UML] Boot broken due to buffer overrun

[Balbir Singh]

mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
a big overrun bug reported on UML bootup.

Reported-by: Vikas K Managutte <vikki.km@gmail.com>
Reported-by: Sarvesh Kumar Lal Das <skldas@gmail.com>
Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 arch/um/drivers/mconsole_kern.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff -puN arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation arch/um/drivers/mconsole_kern.c
--- linux-2.6.28-rc6/arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation	2008-11-29 15:29:04.000000000 +0530
+++ linux-2.6.28-rc6-balbir/arch/um/drivers/mconsole_kern.c	2008-11-29 15:32:23.000000000 +0530
@@ -16,6 +16,8 @@
 #include <linux/slab.h>
 #include <linux/syscalls.h>
 #include <linux/utsname.h>
+#include <linux/socket.h>
+#include <linux/un.h>
 #include <linux/workqueue.h>
 #include <linux/mutex.h>
 #include <asm/uaccess.h>
@@ -785,7 +787,7 @@ static int __init mconsole_init(void)
 	/* long to avoid size mismatch warnings from gcc */
 	long sock;
 	int err;
-	char file[256];
+	char file[UNIX_PATH_MAX];
 
 	if (umid_file_name("mconsole", file, sizeof(file)))
 		return -1;
_

-- 
	Balbir

Re: [PATCH][UML] Boot broken due to buffer overrun

[Pekka Enberg]

Hi Balbir,

On Sat, Nov 29, 2008 at 12:08 PM, Balbir Singh
<balbir@linux.vnet.ibm.com> wrote:
> mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
> the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
> a big overrun bug reported on UML bootup.

Maybe mention that ->sun_path in struct sockaddr_un is UNIX_PATH_MAX
long which causes the problem?

> Reported-by: Vikas K Managutte <vikki.km@gmail.com>
> Reported-by: Sarvesh Kumar Lal Das <skldas@gmail.com>
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>

Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ---
>
>  arch/um/drivers/mconsole_kern.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff -puN arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation arch/um/drivers/mconsole_kern.c
> --- linux-2.6.28-rc6/arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation      2008-11-29 15:29:04.000000000 +0530
> +++ linux-2.6.28-rc6-balbir/arch/um/drivers/mconsole_kern.c     2008-11-29 15:32:23.000000000 +0530
> @@ -16,6 +16,8 @@
>  #include <linux/slab.h>
>  #include <linux/syscalls.h>
>  #include <linux/utsname.h>
> +#include <linux/socket.h>
> +#include <linux/un.h>
>  #include <linux/workqueue.h>
>  #include <linux/mutex.h>
>  #include <asm/uaccess.h>
> @@ -785,7 +787,7 @@ static int __init mconsole_init(void)
>        /* long to avoid size mismatch warnings from gcc */
>        long sock;
>        int err;
> -       char file[256];
> +       char file[UNIX_PATH_MAX];
>
>        if (umid_file_name("mconsole", file, sizeof(file)))
>                return -1;
> _
>
> --
>        Balbir
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH][UML] Boot broken due to buffer overrun

[Balbir Singh]

* Pekka Enberg <penberg@cs.helsinki.fi> [2008-11-29 12:21:46]:

> Hi Balbir,
> 
> On Sat, Nov 29, 2008 at 12:08 PM, Balbir Singh
> <balbir@linux.vnet.ibm.com> wrote:
> > mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
> > the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
> > a big overrun bug reported on UML bootup.
> 
> Maybe mention that ->sun_path in struct sockaddr_un is UNIX_PATH_MAX
> long which causes the problem?
>

Yes, the changelog can definitely add that to the changelog, I'll
resend the patch if Jeff asks for it.

 
> > Reported-by: Vikas K Managutte <vikki.km@gmail.com>
> > Reported-by: Sarvesh Kumar Lal Das <skldas@gmail.com>
> > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> 
> Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

Thanks!

-- 
	Balbir

Re: [PATCH][UML] Boot broken due to buffer overrun

[Andrew Morton]

On Sat, 29 Nov 2008 17:50:39 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * Pekka Enberg <penberg@cs.helsinki.fi> [2008-11-29 12:21:46]:
> 
> > Hi Balbir,
> > 
> > On Sat, Nov 29, 2008 at 12:08 PM, Balbir Singh
> > <balbir@linux.vnet.ibm.com> wrote:
> > > mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
> > > the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
> > > a big overrun bug reported on UML bootup.
> > 
> > Maybe mention that ->sun_path in struct sockaddr_un is UNIX_PATH_MAX
> > long which causes the problem?
> >
> 
> Yes, the changelog can definitely add that to the changelog, I'll
> resend the patch if Jeff asks for it.

Jeff's been quiet lately - I'll merge this one.

I also tagged it for -stable, subject to Jeff's OK.  I'm wondering why
this wasn't observed in earlier kernels?

Re: [PATCH][UML] Boot broken due to buffer overrun

[Américo Wang]

On Tue, Dec 2, 2008 at 8:17 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Sat, 29 Nov 2008 17:50:39 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>
>> * Pekka Enberg <penberg@cs.helsinki.fi> [2008-11-29 12:21:46]:
>>
>> > Hi Balbir,
>> >
>> > On Sat, Nov 29, 2008 at 12:08 PM, Balbir Singh
>> > <balbir@linux.vnet.ibm.com> wrote:
>> > > mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
>> > > the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
>> > > a big overrun bug reported on UML bootup.
>> >
>> > Maybe mention that ->sun_path in struct sockaddr_un is UNIX_PATH_MAX
>> > long which causes the problem?
>> >
>>
>> Yes, the changelog can definitely add that to the changelog, I'll
>> resend the patch if Jeff asks for it.
>
> Jeff's been quiet lately - I'll merge this one.


Yes, indeed.

>
> I also tagged it for -stable, subject to Jeff's OK.  I'm wondering why
> this wasn't observed in earlier kernels?

Maybe because mconsole is not often used for uml? :)

Re: [PATCH][UML] Boot broken due to buffer overrun

[Balbir Singh]

* Andrew Morton <akpm@linux-foundation.org> [2008-12-02 12:17:07]:

> On Sat, 29 Nov 2008 17:50:39 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * Pekka Enberg <penberg@cs.helsinki.fi> [2008-11-29 12:21:46]:
> > 
> > > Hi Balbir,
> > > 
> > > On Sat, Nov 29, 2008 at 12:08 PM, Balbir Singh
> > > <balbir@linux.vnet.ibm.com> wrote:
> > > > mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
> > > > the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
> > > > a big overrun bug reported on UML bootup.
> > > 
> > > Maybe mention that ->sun_path in struct sockaddr_un is UNIX_PATH_MAX
> > > long which causes the problem?
> > >
> > 
> > Yes, the changelog can definitely add that to the changelog, I'll
> > resend the patch if Jeff asks for it.
> 
> Jeff's been quiet lately - I'll merge this one.
> 

Thanks!

> I also tagged it for -stable, subject to Jeff's OK.  I'm wondering why
> this wasn't observed in earlier kernels?
> 

I observed it with the newer toolchain that uses __USE_FORTIFY_LEVEL
and __FORTIFY_SOURCE


-- 
	Balbir

Re: [PATCH][UML] Boot broken due to buffer overrun

[Américo Wang]

On Sat, Nov 29, 2008 at 10:08 AM, Balbir Singh
<balbir@linux.vnet.ibm.com> wrote:
>
> mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
> the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
> a big overrun bug reported on UML bootup.
>
> Reported-by: Vikas K Managutte <vikki.km@gmail.com>
> Reported-by: Sarvesh Kumar Lal Das <skldas@gmail.com>
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
>
>  arch/um/drivers/mconsole_kern.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff -puN arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation arch/um/drivers/mconsole_kern.c
> --- linux-2.6.28-rc6/arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation      2008-11-29 15:29:04.000000000 +0530
> +++ linux-2.6.28-rc6-balbir/arch/um/drivers/mconsole_kern.c     2008-11-29 15:32:23.000000000 +0530
> @@ -16,6 +16,8 @@
>  #include <linux/slab.h>
>  #include <linux/syscalls.h>
>  #include <linux/utsname.h>
> +#include <linux/socket.h>
> +#include <linux/un.h>
>  #include <linux/workqueue.h>
>  #include <linux/mutex.h>
>  #include <asm/uaccess.h>
> @@ -785,7 +787,7 @@ static int __init mconsole_init(void)
>        /* long to avoid size mismatch warnings from gcc */
>        long sock;
>        int err;
> -       char file[256];
> +       char file[UNIX_PATH_MAX];
>


Aha, good spot!

Please also correct the length of mconsole_socket_name.

Anyway,

Reviewed-by: WANG Cong <wangcong@zeuux.org>

Re: [PATCH][UML] Boot broken due to buffer overrun

[Balbir Singh]

* WANG Cong <xiyou.wangcong@gmail.com> [2008-11-29 13:03:42]:

> On Sat, Nov 29, 2008 at 10:08 AM, Balbir Singh
> <balbir@linux.vnet.ibm.com> wrote:
> >
> > mconsole_init() passed 256 bytes as length in os_create_unix_socket, while
> > the sizeof UNIX_PATH_MAX is 108. This patch fixes that problem and avoids
> > a big overrun bug reported on UML bootup.
> >
> > Reported-by: Vikas K Managutte <vikki.km@gmail.com>
> > Reported-by: Sarvesh Kumar Lal Das <skldas@gmail.com>
> > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> > ---
> >
> >  arch/um/drivers/mconsole_kern.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff -puN arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation arch/um/drivers/mconsole_kern.c
> > --- linux-2.6.28-rc6/arch/um/drivers/mconsole_kern.c~fix-uml-buggy-socket-creation      2008-11-29 15:29:04.000000000 +0530
> > +++ linux-2.6.28-rc6-balbir/arch/um/drivers/mconsole_kern.c     2008-11-29 15:32:23.000000000 +0530
> > @@ -16,6 +16,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/utsname.h>
> > +#include <linux/socket.h>
> > +#include <linux/un.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/mutex.h>
> >  #include <asm/uaccess.h>
> > @@ -785,7 +787,7 @@ static int __init mconsole_init(void)
> >        /* long to avoid size mismatch warnings from gcc */
> >        long sock;
> >        int err;
> > -       char file[256];
> > +       char file[UNIX_PATH_MAX];
> >
> 
> 
> Aha, good spot!
> 
> Please also correct the length of mconsole_socket_name.
>

I did not fix that with UNIX_PATH_MAX since, mconsole_user.c already includes
sys/un.h and sys/socket.h and I did not want to include kernel headers
linux/socket.h and linux/un.h there. I could have hard-coded those
values to 108, but I just let it be for now. If you feel strongly
about it, it can be done.
 
> Anyway,
> 
> Reviewed-by: WANG Cong <wangcong@zeuux.org>

Thanks!

-- 
	Balbir

Re: [PATCH][UML] Boot broken due to buffer overrun

[Américo Wang]

On Sun, Nov 30, 2008 at 3:01 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>
> I did not fix that with UNIX_PATH_MAX since, mconsole_user.c already includes
> sys/un.h and sys/socket.h and I did not want to include kernel headers
> linux/socket.h and linux/un.h there. I could have hard-coded those
> values to 108, but I just let it be for now. If you feel strongly
> about it, it can be done.
>

No, you can leave it to me. :)

Thanks.