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:
[email protected] 2004-01-31 21:16:34-02:00 [email protected]
> [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
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
<[email protected]>
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
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
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
<[email protected]>
James Morris <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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?
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
<[email protected]>