Re: [PATCH] NTFS comment expanded, small fix.

Re: [PATCH] NTFS comment expanded, small fix.

[Anton Altaparmakov]

Linus, Alan,

Please do not apply this patch as both the comment and the code are wrong
and unnecessary, respectively.

Can the numerous ntfs fixes in the -ac series be applied to the mainstream
kernel instead? Thanks.

Rogier and everyone doing any NTFS work, please use -ac series kernels as
ntfs has had major updates which are now proven to be a Good Thing
(TM)... - I get bug reports for mainstream kernel ntfs several times a
month while I haven't received a single one for the -ac series (perhaps
due to smaller userbase admittedly, but all bugs reported are fixed in the
-ac patches).

Also, if you need info about ntfs read the ntfs docs at:
	http://linux-ntfs.sourceforge.net/ntfs

For example the fixups are explained at:
	http://linux-ntfs.sourceforge.net/ntfs/concepts/fixup.html

And if the docs don't suffice (they are work in progress), look at the
linux-ntfs project source code.  Especially at the doc directory and the
include directory (and sometimes the libntfs directory). Either the header
files or the library files contain extensive documentation about the
meaning of each and every field in the ntfs structures. For example, the
fixup mechanism is described in: 
	linux-ntfs/include/layout.h, lines 84 to 110.
You can find the most current code in CVS on sourceforge. The project page
is:
	http://sourceforge.net/projects/linux-ntfs/

You can browse the cvs cvs on the web or download the lot. (don't use the
now out of date packaged linux-ntfs-0.0.1 distribution as it is out of
date...)

At 13:53 15/04/2001, Rogier Wolff wrote:
>I am studying an NTFS problem, and came across the NTFS fixup mechanism. 

Care to elaborate? - We could save you some time perhaps...

>It took me much too long to understand the fixup mechanism, even though
a comment tried to explain it. So I rewrote the comment. 

If you read what I referenced above you will want to revise your own
comment....

>Also, the "start" value that is read from the record, could be much 
larger than expected, which could lead to accessing random data. The
fixup should fail then, and this is also patched below.

No it can't (in theory). The volume would be corrupt if it was. That kind
of check belongs in ntfs fsck utility but not in kernel code.

In any case, the correct check, if you want one, would be:

if (start + (count * 2) > size)
	return 0;

And it has to happen before the:

count--;

Hope this helps,

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS maintainer / WWW: http://sourceforge.net/projects/linux-ntfs/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] NTFS comment expanded, small fix.

[Rogier Wolff]

Anton Altaparmakov wrote:
> >Also, the "start" value that is read from the record, could be much 
> larger than expected, which could lead to accessing random data. The
> fixup should fail then, and this is also patched below.
> 
> No it can't (in theory). The volume would be corrupt if it was. That kind
> of check belongs in ntfs fsck utility but not in kernel code.
> 
> In any case, the correct check, if you want one, would be:
> 
> if (start + (count * 2) > size)
> 	return 0;

Hi Anton, 

Of course this is the better check. I was being sloppy. 

I disagree with your "this belongs in an fsck-program". If this
condition triggers, then indeed, the filesystem is corrupt. But if the
"start" pointer is dereferenced, the kernel could be accessing an area
that you don't want touched (e.g. if the buffer happens to be near 
enough to the "end-of-memory", you could "Ooops" .

The kernel should validate all user-input as much as possible, and an
ntfs-formatted-floppy should count as such. 

The "fixup" routine has a bunch of "return 0" conditions. These are
similar to mine: If they trigger, the filesystem must be corrupt.
It's a sanity check, which is neccesary to keep Linux stable.

				Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2137555 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
* There are old pilots, and there are bold pilots. 
* There are also old, bald pilots. 

Re: [PATCH] NTFS comment expanded, small fix.

[Anton Altaparmakov]

On Sun, 15 Apr 2001, Rogier Wolff wrote:
> Anton Altaparmakov wrote:
> > >Also, the "start" value that is read from the record, could be much 
> > larger than expected, which could lead to accessing random data. The
> > fixup should fail then, and this is also patched below.
> > 
> > No it can't (in theory). The volume would be corrupt if it was. That kind
> > of check belongs in ntfs fsck utility but not in kernel code.
> > 
> > In any case, the correct check, if you want one, would be:
> > 
> > if (start + (count * 2) > size)
> > 	return 0;
> 
> Of course this is the better check. I was being sloppy. 
> 
> I disagree with your "this belongs in an fsck-program". If this
> condition triggers, then indeed, the filesystem is corrupt. But if the
> "start" pointer is dereferenced, the kernel could be accessing an area
> that you don't want touched (e.g. if the buffer happens to be near 
> enough to the "end-of-memory", you could "Ooops" .
> 
> The kernel should validate all user-input as much as possible, and an
> ntfs-formatted-floppy should count as such.

Ok. I see your point. But you have to admitt that if it is possible for a
malicious person to gain physicall access to the machine your security is
already zero whatever you do. And I wouldn't expect any sane admin to want
to trash their mashine.

[According to MS of course, NTFS cannot be put on floppies but let's not
get into this discussion.]

> The "fixup" routine has a bunch of "return 0" conditions. These are
> similar to mine: If they trigger, the filesystem must be corrupt.
> It's a sanity check, which is neccesary to keep Linux stable.

Yes, but if you wanted to check for one and every single possibility of
how a filesystem could be trying to kill your fs driver then you driver
will end up with 90% of code being checks in the driver and this
resulting in the driver being slow as hell. If you can't trust your
fs, what can you trust? - That's what the dirty flag is for. If dirty on
mount run fsck to fix corruptions, after that assume that fs is not
corrupt...

But, point taken. I guess we want to safe guard all places where
corruption would result in dereferencing memory outside the buffer (here
ntfs record). It should be safe to ignore all other corruption, from 
this aspect of security.

I will be sending out all my fixes which are in -ac kernels to
Linus soon, myself, and I will include a patch to do a full bounds check
at this point.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS maintainer / WWW: http://sourceforge.net/projects/linux-ntfs/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] NTFS comment expanded, small fix.

[Alan Cox]

> Can the numerous ntfs fixes in the -ac series be applied to the mainstream
> kernel instead? Thanks.

Want me to feed them to Linus or will you do it ?

Re: [PATCH] NTFS comment expanded, small fix.

[Anton Altaparmakov]

At 23:11 15/04/2001, Alan Cox wrote:
> > Can the numerous ntfs fixes in the -ac series be applied to the mainstream
> > kernel instead? Thanks.
>
>Want me to feed them to Linus or will you do it ?

If you have the diffs ready then it would be great if you could do that. 
(Did the maxbytes stuff enter the mainstream kernel yet? Are you going to 
feed them together? Or will that be dropped for now?)

Cheers,

         Anton

PS. If you are too busy let me know and I will do it as soon as I have my 
normal net connection back...


-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://sourceforge.net/projects/linux-ntfs/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] NTFS comment expanded, small fix.

[Alan Cox]

> If you have the diffs ready then it would be great if you could do that. 
> (Did the maxbytes stuff enter the mainstream kernel yet? Are you going to 
> feed them together? Or will that be dropped for now?)

maxbytes got to Linus. The checks using it are not all there, but maxbytes
went in early to avoid fs differences being hard to maintain. I've fed him
bits of the fs checking this time. There is some negotiation to be done over
truncate yet

[PATCH] NTFS comment expanded, small fix.

[Rogier Wolff]

Hi all,

I am studying an NTFS problem, and came across the NTFS fixup mechanism. 

It took me much too long to understand the fixup mechanism, even though
a comment tried to explain it. So I rewrote the comment. 

Also, the "start" value that is read from the record, could be much 
larger than expected, which could lead to accessing random data. The
fixup should fail then, and this is also patched below. 

Patch attached. 

				Roger. 

--------------------------------------------------------------------

diff -ur linux-2.4.3.clean/fs/ntfs/super.c linux-2.4.3.ntfs_fix/fs/ntfs/super.c
--- linux-2.4.3.clean/fs/ntfs/super.c	Sun Apr 15 14:48:05 2001
+++ linux-2.4.3.ntfs_fix/fs/ntfs/super.c	Sun Apr 15 14:47:48 2001
@@ -30,6 +30,22 @@
  * . the magic identifier is wrong
  * . the size is given and does not match the number of sectors
  * . a fixup is invalid
+ ******
+ * Somehow that comment may sound usable to the person who wrote it, but 
+ * in fact it took me over an hour to figure it out. That's not what 
+ * comments are for. So let me try to explain it: 
+ *
+ * A record contains a fixup-area. The size of this area is S+1 words,
+ * with S the number of sectors in the record. 
+ *
+ * The first word of the fixup area is a random word. 
+ * The last word of every sector should contain this random word. 
+ * The rest of the fixup area contains the original contents of that
+ * last word of each sector of the record. 
+ * the position and length of the fixup area are stored at offset 4 
+ * and 6 in the record.  
+ *
+ * Hope this helps. -- REW
  */
 int ntfs_fixup_record(ntfs_volume *vol, char *record, char *magic, int size)
 {
@@ -42,6 +58,8 @@
 	count=NTFS_GETU16(record+6);
 	count--;
 	if(size && vol->blocksize*count != size)
+		return 0;
+	if (start >= size) 
 		return 0;
 	fixup = NTFS_GETU16(record+start);
 	start+=2;
Only in linux-2.4.3.ntfs_fix/fs/ntfs: super.c.orig


-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2137555 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
* There are old pilots, and there are bold pilots. 
* There are also old, bald pilots.