Mysterious string truncation in 2.4.25 kernel

Mysterious string truncation in 2.4.25 kernel

[Glen Nakamura]

Aloha,

I'm running a patched 2.4.25 kernel and I noticed truncated strings in
various procfs files.  e.g.

# cat /proc/tty/drivers
se                   /dev/cua/%d     5  64-127 serial:callout
se                   /dev/tts/%d     4  64-127 se
pty_slave            /dev/pts/%d   136   0-255 pty:slave
pty_master           /dev/ptm      128   0-255 pty:master
pty_slave            /dev/pty/s%d    3   0-255 pty:slave
pty_master           /dev/pty/m%d    2   0-255 pty:master
/dev/vc/0            /dev/vc/0       4       0 system:vtmaster
/dev/ptmx            /dev/ptmx       5       2 system
/dev/console         /dev/console    5       1 system:console
/dev/tty             /dev/tty        5       0 system:/dev/tty
unknown              /dev/vc/%d      4    1-63 console

Notice that "serial" is output as "se" in the top two lines.
I was able to produce similar results on a vanilla 2.4.25 kernel by
simply changing the length of a few strings in fs/proc/proc_tty.c.
I believe the problem is caused by the following patch:

ChangeSet@1.1290.1.16  2004-01-31 21:16:34-02:00  jmorris@redhat.com

> [PATCH] Zero last byte of mount option page
> 
> Hi Al,
> 
> Here's a patch which zeroes the last byte of the mount option data copied
> from userspace during mount(2).
> 
> For filesystems which parse mount options as strings (the majority), lack
> of a zero terminator could cause the page to be overrun.  The source code
> comments specify that the maximum size of the mount data is PAGE_SIZE-1,
> so this patch will not affect any valid binary-formatted mount data.

--- 1.24/fs/namespace.c	Tue Mar  2 15:50:11 2004
+++ 1.25/fs/namespace.c	Tue Mar  2 15:50:11 2004
@@ -715,6 +715,9 @@
 	if (dev_name && !memchr(dev_name, 0, PAGE_SIZE))
 		return -EINVAL;
 
+	if (data_page)
+		((char *)data_page)[PAGE_SIZE - 1] = 0;
+
 	/* Separate the per-mountpoint flags */
 	if (flags & MS_NOSUID)
 		mnt_flags |= MNT_NOSUID;

Could someone please comment on the correctness of the above patch
especially regarding procfs?

- glen

Re: Mysterious string truncation in 2.4.25 kernel

[James Morris]

On Tue, 2 Mar 2004, Glen Nakamura wrote:

> Could someone please comment on the correctness of the above patch
> especially regarding procfs?

I don't see how the patch could be related to the problem you are seeing.  


- James
-- 
James Morris
<jmorris@redhat.com>

Re: Mysterious string truncation in 2.4.25 kernel

[Glen Nakamura]

On Tue, Mar 02, 2004 at 11:03:15PM -0500, James Morris wrote:
> I don't see how the patch could be related to the problem you are seeing.  

Thanks for the response...  I took another look and my current theory is
that the problem occurs in the following invocation of do_mount:

void __init mount_devfs_fs (void)
{
    int err;
                                                                                
    if ( !(boot_options & OPTION_MOUNT) ) return;
    if (err == 0) printk (KERN_INFO "Mounted devfs on /dev\n");
    else PRINTK ("(): unable to mount devfs, err: %d\n", err);
}   /*  End Function mount_devfs_fs  */

This call to do_mount is on line 3552 of fs/devfs/base.c and passes a const
string as the data_page parameter.  Then in do_mount in fs/namespace.c on
line 718:

	if (data_page)
		((char *)data_page)[PAGE_SIZE - 1] = 0;

The above statement zeros a byte that is out of bounds and corrupts another
string in the same section of memory.  In my build, this happens to truncate
the "serial" string to "se".

So is it really safe to simply zero the byte at [PAGE_SIZE - 1]?
The comment says "up to PAGE_SIZE-1 bytes", _not_ "exactly PAGE_SIZE-1 bytes".
It doesn't mention anything about padding, etc.

Of course, perhaps 0 should passed instead of "" for data_page?

-    err = do_mount ("none", "/dev", "devfs", 0, "");
+    err = do_mount ("none", "/dev", "devfs", 0, 0);

Comments?

- glen

Re: Mysterious string truncation in 2.4.25 kernel

[Glen Nakamura]

On Tue, Mar 02, 2004 at 07:35:47PM -1000, Glen Nakamura wrote:
> void __init mount_devfs_fs (void)
> {
>     int err;
> 
>     if ( !(boot_options & OPTION_MOUNT) ) return;
>+    err = do_mount ("none", "/dev", "devfs", 0, "");
>     if (err == 0) printk (KERN_INFO "Mounted devfs on /dev\n");
>     else PRINTK ("(): unable to mount devfs, err: %d\n", err);
> }   /*  End Function mount_devfs_fs  */

Whoops... I deleted the do_mount line by accident.

- glen

Re: Mysterious string truncation in 2.4.25 kernel

[James Morris]

On Tue, 2 Mar 2004, Glen Nakamura wrote:

> Of course, perhaps 0 should passed instead of "" for data_page?
> 
> -    err = do_mount ("none", "/dev", "devfs", 0, "");
> +    err = do_mount ("none", "/dev", "devfs", 0, 0);
>
> Comments?

Yes, the devfs fix above is needed if the data_page patch has been 
applied.  

This is the case in 2.6, but not 2.4.25.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: Mysterious string truncation in 2.4.25 kernel

[Herbert Xu]

James Morris <jmorris@redhat.com> wrote:
> On Tue, 2 Mar 2004, Glen Nakamura wrote:
> 
>> Of course, perhaps 0 should passed instead of "" for data_page?
>> 
>> -    err = do_mount ("none", "/dev", "devfs", 0, "");
>> +    err = do_mount ("none", "/dev", "devfs", 0, 0);
>>
>> Comments?
> 
> Yes, the devfs fix above is needed if the data_page patch has been 
> applied.  
> 
> This is the case in 2.6, but not 2.4.25.

Hmm the data_page line is in my copy of 2.4.25...
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Mysterious string truncation in 2.4.25 kernel

[Marcelo Tosatti]

On Wed, 3 Mar 2004, James Morris wrote:

> On Tue, 2 Mar 2004, Glen Nakamura wrote:
> 
> > Of course, perhaps 0 should passed instead of "" for data_page?
> > 
> > -    err = do_mount ("none", "/dev", "devfs", 0, "");
> > +    err = do_mount ("none", "/dev", "devfs", 0, 0);
> >
> > Comments?
> 
> Yes, the devfs fix above is needed if the data_page patch has been 
> applied.  
> 
> This is the case in 2.6, but not 2.4.25.

My bad :(

Changed last argument of fs/devfs/base.c do_mount() call to NULL, as per 
2.6.

James, wonder if your change can't cause other similar problems in 2.4? 

Re: Mysterious string truncation in 2.4.25 kernel

[James Morris]

On Wed, 3 Mar 2004, Marcelo Tosatti wrote:

> 
> 
> On Wed, 3 Mar 2004, James Morris wrote:
> 
> > On Tue, 2 Mar 2004, Glen Nakamura wrote:
> > 
> > > Of course, perhaps 0 should passed instead of "" for data_page?
> > > 
> > > -    err = do_mount ("none", "/dev", "devfs", 0, "");
> > > +    err = do_mount ("none", "/dev", "devfs", 0, 0);
> > >
> > > Comments?
> > 
> > Yes, the devfs fix above is needed if the data_page patch has been 
> > applied.  
> > 
> > This is the case in 2.6, but not 2.4.25.
> 
> My bad :(
> 
> Changed last argument of fs/devfs/base.c do_mount() call to NULL, as per 
> 2.6.
> 
> James, wonder if your change can't cause other similar problems in 2.4? 

Looks like sunos_nfs_mount() needs to be fixed to pass a page as the last 
argument of do_mount().


- James
-- 
James Morris
<jmorris@redhat.com>