2006-10-17 14:48:41

by Joerg Schilling

[permalink] [raw]
Subject: Linux ISO-9660 Rock Ridge bug needs fix

Hi,

while working on better ISO-9660 support for the Solaris Kernel,
I recently enhanced mkisofs to support the Rock Ridge Standard version 1.12
from 1994.

The difference bewteen version 1.12 and 1.10 (this is what previous
mkisofs versions did implement) is that the "PX" field is now 8 Byte
bigger than before (44 instead of 36 bytes).

As Rock Ridge is a protocol that implements a list of size tagged fields,
this change in mkisofs should not be a problem and in fact is not for Solaris
or FreeBSD. As Linux does not implement Rock Rige correctly, Linux will
reject CDs/DVDs that have been created by a recent mkisofs.

As Linux will completely disable RR because of this bug, it must be called
a showstopper bug that needs immediate fixing and that also needs to be
backported.

The recent version of cdrtools that include the new mkisofs is located at:

ftp://ftp.berlios.de/pub/cdrecord/alpha/cdrtools-2.01.01a18-pre.tar.bz2

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily


2006-10-17 17:41:37

by Ismail Dönmez

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

17 Eki 2006 Sal 17:45 tarihinde, Joerg Schilling şunları yazmıştı:
> Hi,
>
> while working on better ISO-9660 support for the Solaris Kernel,
> I recently enhanced mkisofs to support the Rock Ridge Standard version 1.12
> from 1994.
>
> The difference bewteen version 1.12 and 1.10 (this is what previous
> mkisofs versions did implement) is that the "PX" field is now 8 Byte
> bigger than before (44 instead of 36 bytes).

Is there a test iso file somewhere? I think the attached *untested* patch will
fix it.

Regards,
ismail


Attachments:
(No filename) (535.00 B)
rock.patch (644.00 B)
Download all attachments

2006-10-17 18:01:52

by Luca Tettamanti

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Ismail Donmez <[email protected]> ha scritto:
>>
>> while working on better ISO-9660 support for the Solaris Kernel,
>> I recently enhanced mkisofs to support the Rock Ridge Standard version 1.12
>> from 1994.
>>
>> The difference bewteen version 1.12 and 1.10 (this is what previous
>> mkisofs versions did implement) is that the "PX" field is now 8 Byte
>> bigger than before (44 instead of 36 bytes).
>
> Is there a test iso file somewhere? I think the attached *untested* patch will
> fix it.

I was also looking at this ;) I cannot reproduce the failure even with
images generated with the new version of mkisofs (I actually _see_ that
PX record size is changed, but isofs doesn't seem to care...).

> diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
> index f3a1db3..061a633 100644
> --- a/fs/isofs/rock.c
> +++ b/fs/isofs/rock.c
> @@ -349,6 +349,7 @@ #endif
> inode->i_nlink = isonum_733(rr->u.PX.n_links);
> inode->i_uid = isonum_733(rr->u.PX.uid);
> inode->i_gid = isonum_733(rr->u.PX.gid);
> + inode->i_ino = isonum_733(rr->u.PX.ino);
> break;

I don't think it's correct. When reading disk with old format i_ino will
be filled with garbage.
Now, who is in charge of isofs?

Signed-off-by: Luca Tettamanti <[email protected]>

---
fs/isofs/rock.c | 4 ++++
fs/isofs/rock.h | 1 +
2 files changed, 5 insertions(+)

diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
index f3a1db3..9a87010 100644
--- a/fs/isofs/rock.c
+++ b/fs/isofs/rock.c
@@ -349,6 +349,10 @@ #endif
inode->i_nlink = isonum_733(rr->u.PX.n_links);
inode->i_uid = isonum_733(rr->u.PX.uid);
inode->i_gid = isonum_733(rr->u.PX.gid);
+
+ /* Rock Ridge V1.12, override inode number */
+ if (rr->len == 44)
+ inode->i_ino = isonum_733(rr->u.PX.inode);
break;
case SIG('P', 'N'):
{
diff --git a/fs/isofs/rock.h b/fs/isofs/rock.h
index ed09e2b..df5f2a7 100644
--- a/fs/isofs/rock.h
+++ b/fs/isofs/rock.h
@@ -33,6 +33,7 @@ struct RR_PX_s {
char n_links[8];
char uid[8];
char gid[8];
+ char inode[8];
};

struct RR_PN_s {


Luca
--
Sbagliare ? umano, ma per incasinare davvero le cose serve un computer.

2006-10-17 18:14:24

by Ismail Dönmez

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

17 Eki 2006 Sal 21:02 tarihinde, Luca Tettamanti şunları yazmıştı:
> Ismail Donmez <[email protected]> ha scritto:
> >> while working on better ISO-9660 support for the Solaris Kernel,
> >> I recently enhanced mkisofs to support the Rock Ridge Standard version
> >> 1.12 from 1994.
> >>
> >> The difference bewteen version 1.12 and 1.10 (this is what previous
> >> mkisofs versions did implement) is that the "PX" field is now 8 Byte
> >> bigger than before (44 instead of 36 bytes).
> >
> > Is there a test iso file somewhere? I think the attached *untested* patch
> > will fix it.
>
> I was also looking at this ;) I cannot reproduce the failure even with
> images generated with the new version of mkisofs (I actually _see_ that
> PX record size is changed, but isofs doesn't seem to care...).
>
> > diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
> > index f3a1db3..061a633 100644
> > --- a/fs/isofs/rock.c
> > +++ b/fs/isofs/rock.c
> > @@ -349,6 +349,7 @@ #endif
> > inode->i_nlink = isonum_733(rr->u.PX.n_links);
> > inode->i_uid = isonum_733(rr->u.PX.uid);
> > inode->i_gid = isonum_733(rr->u.PX.gid);
> > + inode->i_ino = isonum_733(rr->u.PX.ino);
> > break;
>
> I don't think it's correct. When reading disk with old format i_ino will
> be filled with garbage.
> Now, who is in charge of isofs?

I was just trying a fast hack to see it works ;-) but iso files produced by
latest mkisofs works fine even without patching.

Regards,
ismail

2006-10-17 18:16:48

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Ismail Donmez <[email protected]> wrote:

> 17 Eki 2006 Sal 17:45 tarihinde, Joerg Schilling ??unları yazmı??tı:
> > Hi,
> >
> > while working on better ISO-9660 support for the Solaris Kernel,
> > I recently enhanced mkisofs to support the Rock Ridge Standard version 1.12
> > from 1994.
> >
> > The difference bewteen version 1.12 and 1.10 (this is what previous
> > mkisofs versions did implement) is that the "PX" field is now 8 Byte
> > bigger than before (44 instead of 36 bytes).
>
> Is there a test iso file somewhere? I think the attached *untested* patch will
> fix it.

Well, this is why I did offer a preliminary version of thelatest mkisofs
sources.....


But note: your patch does not fix the original implementation bug and it is most
unlikely that the hack will do the right things in all cases.

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-17 18:22:51

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Ismail Donmez <[email protected]> wrote:

> I was just trying a fast hack to see it works ;-) but iso files produced by
> latest mkisofs works fine even without patching.

Did you _really_ use the latest mkisofs?

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-17 18:36:00

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Ismail Donmez <[email protected]> wrote:

> > Well, this is why I did offer a preliminary version of thelatest mkisofs
> > sources.....
>
> Well a simple mkisofs some_file > test.iso and mounting that on a loop device
> worked fine.
>
>
> > But note: your patch does not fix the original implementation bug and it is
> > most unlikely that the hack will do the right things in all cases.
>
> Well I don't know whats the original implementation bug and rock.c seems to be
> pretty much old with no active maintainer.

Please read again my original mail....

1) you need to create images with Rock Ridge

2) a correct implementation is prepared to deal with more recent versions
without a need for new changes.

So, if the implementation does not deal with the new version _without_
explicitely knowing about v1.12 it is still broken.



J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-17 18:39:20

by Ismail Dönmez

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

17 Eki 2006 Sal 21:32 tarihinde, Joerg Schilling şunları yazmıştı:
> Ismail Donmez <[email protected]> wrote:
> > > Well, this is why I did offer a preliminary version of thelatest
> > > mkisofs sources.....
> >
> > Well a simple mkisofs some_file > test.iso and mounting that on a loop
> > device worked fine.
> >
> > > But note: your patch does not fix the original implementation bug and
> > > it is most unlikely that the hack will do the right things in all
> > > cases.
> >
> > Well I don't know whats the original implementation bug and rock.c seems
> > to be pretty much old with no active maintainer.
>
> Please read again my original mail....
>
> 1) you need to create images with Rock Ridge

Well tried -force-rr and it didn't generate Rock Ridge either.

> 2) a correct implementation is prepared to deal with more recent versions
> without a need for new changes.
>
> So, if the implementation does not deal with the new version _without_
> explicitely knowing about v1.12 it is still broken.

I am not sure how is this possible, maybe should check how OpenSolaris does
this. No time for that now.

Regards,
ismail

2006-10-17 18:38:06

by James Lamanna

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix


Joerg Schilling wrote:
> Hi,
>
> while working on better ISO-9660 support for the Solaris Kernel,
> I recently enhanced mkisofs to support the Rock Ridge Standard version 1.12
> from 1994.
>
> The difference bewteen version 1.12 and 1.10 (this is what previous
> mkisofs versions did implement) is that the "PX" field is now 8 Byte
> bigger than before (44 instead of 36 bytes).
>
> As Rock Ridge is a protocol that implements a list of size tagged fields,
> this change in mkisofs should not be a problem and in fact is not for Solaris
> or FreeBSD. As Linux does not implement Rock Rige correctly, Linux will
> reject CDs/DVDs that have been created by a recent mkisofs.
>
> As Linux will completely disable RR because of this bug, it must be called
> a showstopper bug that needs immediate fixing and that also needs to be
> backported.

Hi Joerg,

It looks like Linux would definitely have an issue with field sizes changing
because it does an overflow calculation (see rock_check_overflow())
based on struct sizes.
Have you seen the error that Linux generates? If so please post it. I'm
assuming its probably something along the lines of:
"rock: directory entry would overflow storage...."

I've attached a patch that may fix the problem for now.
This is only compile-tested, not run tested or tested against any
RockRidge 1.12 images yet. Unfortunately I'm not by any machines with any
flavor of linux or cdrecord that actually have a CD burner I can test this on.

As I am not any sort of ISOFS maintainer by any stretch of the imagination all
review is welcome.

Thanks.

-- James Lamanna

Add a PX entry structure that includes the File Serial Number field per
RockRidge version 1.12.

Signed-off-by: James Lamanna <[email protected]>
---
diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
index f3a1db3..48be069 100644
--- a/fs/isofs/rock.c
+++ b/fs/isofs/rock.c
@@ -4,6 +4,9 @@
* (C) 1992, 1993 Eric Youngdale
*
* Rock Ridge Extensions to iso9660
+ *
+ * James Lamanna : Support v. 1.12 PX Entry
+ * ([email protected]) : 17th Oct 2006
*/

#include <linux/slab.h>
@@ -148,8 +151,14 @@ static int rock_check_overflow(struct ro
len = sizeof(struct RR_RR_s);
break;
case SIG('P', 'X'):
- len = sizeof(struct RR_PX_s);
+ {
+ struct rock_ridge *rr = (struct rock_ridge *)rs->chr;
+ if (rr->len == PX_112_LEN)
+ len = sizeof(struct RR_PX_112_s);
+ else
+ len = sizeof(struct RR_PX_s);
break;
+ }
case SIG('P', 'N'):
len = sizeof(struct RR_PN_s);
break;
@@ -349,6 +358,9 @@ #endif
inode->i_nlink = isonum_733(rr->u.PX.n_links);
inode->i_uid = isonum_733(rr->u.PX.uid);
inode->i_gid = isonum_733(rr->u.PX.gid);
+
+ if (rr->len == PX_112_LEN)
+ inode->i_ino = isonum_733(rr->u.PX_112.serial);
break;
case SIG('P', 'N'):
{
diff --git a/fs/isofs/rock.h b/fs/isofs/rock.h
index ed09e2b..fc4478f 100644
--- a/fs/isofs/rock.h
+++ b/fs/isofs/rock.h
@@ -35,6 +35,16 @@ struct RR_PX_s {
char gid[8];
};

+/* RR 1.12 extends the PX entry with a POSIX File Serial Number */
+#define PX_112_LEN (sizeof(struct RR_PX_112_s) + offsetof(struct rock_ridge, u))
+struct RR_PX_112_s {
+ char mode[8];
+ char n_links[8];
+ char uid[8];
+ char gid[8];
+ char serial[8];
+};
+
struct RR_PN_s {
char dev_high[8];
char dev_low[8];
@@ -102,6 +112,7 @@ struct rock_ridge {
struct SU_ER_s ER;
struct RR_RR_s RR;
struct RR_PX_s PX;
+ struct RR_PX_112_s PX_112;
struct RR_PN_s PN;
struct RR_SL_s SL;
struct RR_NM_s NM;

2006-10-17 18:37:22

by Ismail Dönmez

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

17 Eki 2006 Sal 21:16 tarihinde, Joerg Schilling şunları yazmıştı:
> Ismail Donmez <[email protected]> wrote:
> > I was just trying a fast hack to see it works ;-) but iso files produced
> > by latest mkisofs works fine even without patching.
>
> Did you _really_ use the latest mkisofs?

Yes :

[~]> ./mkisofs MeGUI-x264_generic_profiles_v31.7z > test.iso
Total translation table size: 0
Total rockridge attributes bytes: 0
Total directory bytes: 0
Path table size(bytes): 10
Max brk space used 21000
176 extents written (0 MB)

[~]> sudo mount -o loop -t iso9660 test.iso ./test

[~]> ls test
megui_x2.7z

[~]> ./mkisofs --version
mkisofs 2.01.01a18 (i686-pc-linux-gnu)

2006-10-17 19:38:46

by Pekka Enberg

[permalink] [raw]
Subject: Re: Re: Linux ISO-9660 Rock Ridge bug needs fix

On 10/17/06, Luca Tettamanti <[email protected]> wrote:
> Now, who is in charge of isofs?

Try fsdevel and/or akpm.

Pekka

2006-10-17 19:40:58

by Ismail Dönmez

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

17 Eki 2006 Sal 21:12 tarihinde, Joerg Schilling şunları yazmıştı:
> Ismail Donmez <[email protected]> wrote:
> > 17 Eki 2006 Sal 17:45 tarihinde, Joerg Schilling Å?unları yazmıÅ?tı:
> > > Hi,
> > >
> > > while working on better ISO-9660 support for the Solaris Kernel,
> > > I recently enhanced mkisofs to support the Rock Ridge Standard version
> > > 1.12 from 1994.
> > >
> > > The difference bewteen version 1.12 and 1.10 (this is what previous
> > > mkisofs versions did implement) is that the "PX" field is now 8 Byte
> > > bigger than before (44 instead of 36 bytes).
> >
> > Is there a test iso file somewhere? I think the attached *untested* patch
> > will fix it.
>
> Well, this is why I did offer a preliminary version of thelatest mkisofs
> sources.....

Well a simple mkisofs some_file > test.iso and mounting that on a loop device
worked fine.


> But note: your patch does not fix the original implementation bug and it is
> most unlikely that the hack will do the right things in all cases.

Well I don't know whats the original implementation bug and rock.c seems to be
pretty much old with no active maintainer.

Regards,
ismail

2006-10-17 19:50:06

by Luca Tettamanti

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Il Tue, Oct 17, 2006 at 08:16:07PM +0200, Joerg Schilling ha scritto:
> Ismail Donmez <[email protected]> wrote:
>
> > I was just trying a fast hack to see it works ;-) but iso files produced by
> > latest mkisofs works fine even without patching.
>
> Did you _really_ use the latest mkisofs?

Yes, of course. As I said, the size of PX record is different:

000b820: 0100 5350 0701 beef 0052 5205 0181 5058 ..SP.....RR...PX
000b830: 2c01 6d41 0000 0000 416d 0200 0000 0000 ,.mA....Am......
^^ size is 44

but isofs (I'm using 2.6.19-rc2) doesn't care. If I'm reading the code
correctly record size is validated against (dentry size - name len -
records already parsed); it may be possibile to trigger the failure with
a certain combination of records (directory relocation?).
With my patch it should never happens that expected attributes size is
greater than dentry size.
Anyway, if you have a (small) image that triggers the error I can double
check the code.


Luca
--
#include <stdio.h>
int main(void) {printf("\t\t\b\b\b\b\b\b");
printf("\t\t\b\b\b\b\b\b");return 0;}

2006-10-17 19:54:55

by Jan Engelhardt

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

>> > Is there a test iso file somewhere? I think the attached *untested* patch
>> > will fix it.
>>
>> Well, this is why I did offer a preliminary version of thelatest mkisofs
>> sources.....
>
>Well a simple mkisofs some_file > test.iso and mounting that on a loop device
>worked fine.

Hmm, rock ridge is generated by using `mkisofs -r` or `mkisofs -R`. It
is, as far as I can see, not implicitly enabled.


-`J'
--

2006-10-17 21:07:28

by James Lamanna

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix


Joerg Shilling wrote:
> Ismail Donmez <[email protected]> wrote:
>
> > > Well, this is why I did offer a preliminary version of thelatest mkisofs
> > > sources.....
> >
> > Well a simple mkisofs some_file > test.iso and mounting that on a loop
> > device
> > worked fine.
> >
> >
> > > But note: your patch does not fix the original implementation bug and it
> > > is
> > > most unlikely that the hack will do the right things in all cases.
> >
> > Well I don't know whats the original implementation bug and rock.c seems to
> > be
> > pretty much old with no active maintainer.
>
> Please read again my original mail....
> 1) you need to create images with Rock Ridge
>
> 2) a correct implementation is prepared to deal with more recent versions
> without a need for new changes.
>
> So, if the implementation does not deal with the new version _without_
> explicitely knowing about v1.12 it is still broken.

Hi Joerg,

I am unable to duplicate this bug that supposedly exists even on older
kernels.
For instance, on a 2.6.16 kernel I do the following:

$ mkisofs -R -v -o rrtest.iso testiso
mkisofs 2.01.01a18 (i686-pc-linux-gnu)
Scanning testiso
Scanning testiso/d3
Scanning testiso/f2
Writing: Initial Padblock Start Block 0
Done with: Initial Padblock Block(s) 16
Writing: Primary Volume Descriptor Start Block 16
Done with: Primary Volume Descriptor Block(s) 1
Writing: End Volume Descriptor Start Block 17
Done with: End Volume Descriptor Block(s) 1
Writing: Version block Start Block 18
Done with: Version block Block(s) 1
Writing: Path table Start Block 19
Done with: Path table Block(s) 4
Writing: Directory tree Start Block 23
Done with: Directory tree Block(s) 3
Writing: Directory tree cleanup Start Block 26
Done with: Directory tree cleanup Block(s) 0
Writing: Extension record Start Block 26
Done with: Extension record Block(s) 1
Writing: The File(s) Start Block 27
Total translation table size: 0
Total rockridge attributes bytes: 1163
Total directory bytes: 4096
Path table size(bytes): 30
Done with: The File(s) Block(s) 2752
Writing: Ending Padblock Start Block 2779
Done with: Ending Padblock Block(s) 150
Max brk space used 21000
2929 extents written (5 MB)

$ mount rrtest.iso testmount -t iso9660 -o loop=/dev/loop0

Examining testmount/ I find everything there that should be there.
dmesg even reports:
ISO 9660 Extensions: RRIP_1991A

So it is indeed using RockRidge of some sort.
(If there is something I'm doing wrong here, please point it out so I can find
this bug).

I do agree that any implementation should not need to know about the new
version in order to function in 1.10 mode. However, in order to support the
new fields, in this case assigning inode->i_ino from the new PX entry field, it
must know that the record is a v. 1.12 one.

Thanks.

-- James

2006-10-17 21:35:37

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

James Lamanna <[email protected]> wrote:

>
> Joerg Shilling wrote:
> > Ismail Donmez <[email protected]> wrote:
> >
> > > > Well, this is why I did offer a preliminary version of thelatest mkisofs
> > > > sources.....
> > >
> > > Well a simple mkisofs some_file > test.iso and mounting that on a loop
> > > device
> > > worked fine.
> > >
> > >
> > > > But note: your patch does not fix the original implementation bug and it
> > > > is
> > > > most unlikely that the hack will do the right things in all cases.
> > >
> > > Well I don't know whats the original implementation bug and rock.c seems to
> > > be
> > > pretty much old with no active maintainer.
> >
> > Please read again my original mail....
> > 1) you need to create images with Rock Ridge
> >
> > 2) a correct implementation is prepared to deal with more recent versions
> > without a need for new changes.
> >
> > So, if the implementation does not deal with the new version _without_
> > explicitely knowing about v1.12 it is still broken.
>
> Hi Joerg,
>
> I am unable to duplicate this bug that supposedly exists even on older
> kernels.
> For instance, on a 2.6.16 kernel I do the following:

Mm, I did not test, I did only check the source and it seems that I did
interpret the check

len += offsetof(struct rock_ridge, u);
if (len > rs->len) {
printk(KERN_NOTICE "rock: directory entry would overflow "
"storage\n");
printk(KERN_NOTICE "rock: sig=0x%02x, size=%d, remaining=%d\n",
sig, len, rs->len);
return -EIO;
}

the wrong way.... because the error text is wrong. It should be corrected into

"rock: directory entry would _underflow_ storage\n"


Using the inode field from RRip 1.12 is definitely not trivial as it may affect
many parts of the source and needs intensive testing.

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-17 21:52:03

by James Lamanna

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

On 10/17/06, Joerg Schilling <[email protected]> wrote:
> James Lamanna <[email protected]> wrote:
>
> >
> > Joerg Shilling wrote:
[snip...]
> >
> > Hi Joerg,
> >
> > I am unable to duplicate this bug that supposedly exists even on older
> > kernels.
> > For instance, on a 2.6.16 kernel I do the following:
>
> Mm, I did not test, I did only check the source and it seems that I did
> interpret the check
>
> len += offsetof(struct rock_ridge, u);
> if (len > rs->len) {
> printk(KERN_NOTICE "rock: directory entry would overflow "
> "storage\n");
> printk(KERN_NOTICE "rock: sig=0x%02x, size=%d, remaining=%d\n",
> sig, len, rs->len);
> return -EIO;
> }
>
> the wrong way.... because the error text is wrong. It should be corrected into
>
> "rock: directory entry would _underflow_ storage\n"

Yes I saw this check and misinterpreted it initially also.

I actually think 'overflow' is still correct since its testing for the
calcuated size (directory entry) being larger than the size reported
by the filesystem (storage).

I still submit my patch to Linus et. al. for consideration that also
detects overflow in the case of a v 1.12 PX entry. I may have time to
build a git kernel today or tomorrow and actually test it against a RR
iso image.

>
>
> Using the inode field from RRip 1.12 is definitely not trivial as it may affect
> many parts of the source and needs intensive testing.

Yes. If it is actually correct it allows for the use of iget_locked()
in isofs/inode.c instead of iget5_locked() (per
Documentation/filesystems/vfs.txt). Though I'll let a real VFS person
decide if that has any advantages.

-- James

2006-10-17 22:27:34

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

"James Lamanna" <[email protected]> wrote:

> > the wrong way.... because the error text is wrong. It should be corrected into
> >
> > "rock: directory entry would _underflow_ storage\n"
>
> Yes I saw this check and misinterpreted it initially also.
>
> I actually think 'overflow' is still correct since its testing for the
> calcuated size (directory entry) being larger than the size reported
> by the filesystem (storage).
>
> I still submit my patch to Linus et. al. for consideration that also
> detects overflow in the case of a v 1.12 PX entry. I may have time to
> build a git kernel today or tomorrow and actually test it against a RR
> iso image.

If you test for the case that the on disk structure is bigger than expected,
then you break the RR standard.


> > Using the inode field from RRip 1.12 is definitely not trivial as it may affect
> > many parts of the source and needs intensive testing.
>
> Yes. If it is actually correct it allows for the use of iget_locked()
> in isofs/inode.c instead of iget5_locked() (per
> Documentation/filesystems/vfs.txt). Though I'll let a real VFS person
> decide if that has any advantages.

This is not true, the inode numbe is not sufficient to identify a file.

But if you are not a fs expert, you should not continue....

Maging this change work for trivial cases will take an hour, making it work
for the non-obvious cases may take more than a week.

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-17 23:04:27

by Alan

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Ar Mer, 2006-10-18 am 00:24 +0200, ysgrifennodd Joerg Schilling:
> This is not true, the inode numbe is not sufficient to identify a file.
>
> But if you are not a fs expert, you should not continue....

And pray how does someone become an expert ? By doing things and by
learning as you do them. Carry on and ignore Mr Negativity please.

Alan

2006-10-17 23:13:06

by Bodo Eggert

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Luca Tettamanti <[email protected]> wrote:

> +++ b/fs/isofs/rock.c

> +
> + /* Rock Ridge V1.12, override inode number */
> + if (rr->len == 44)
> + inode->i_ino = isonum_733(rr->u.PX.inode);

I think it's wrong again, it will break as soon as rockridge 1.13 defines
an additional field. s,==,>=, should do the trick.

BTW and without digging in the code: Since version 2 will be binary
incompatible, is there a version check?

BTW2, Just to be cautionous: what will happen if somebody forces the same
inode number on two different entries?
--
Ich danke GMX daf?r, die Verwendung meiner Adressen mittels per SPF
verbreiteten L?gen zu sabotieren.

http://david.woodhou.se/why-not-spf.html

2006-10-18 15:10:37

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

[email protected] (Joerg Schilling) wrote:

> > > Using the inode field from RRip 1.12 is definitely not trivial as it may affect
> > > many parts of the source and needs intensive testing.
> >
> > Yes. If it is actually correct it allows for the use of iget_locked()
> > in isofs/inode.c instead of iget5_locked() (per
> > Documentation/filesystems/vfs.txt). Though I'll let a real VFS person
> > decide if that has any advantages.
>
> This is not true, the inode number is not sufficient to identify a file.
>
> But if you are not a fs expert, you should not continue....
>
> Making this change work for trivial cases will take an hour, making it work
> for the non-obvious cases may take more than a week.

The full set of feature enhancements for Linux was to also provide "inode
numnbers" in vanilla ISO-9660 mode and to add a fingerprint into the image so
that the kernel is able to detect this.

Implementing support for the new inode features is supporting this mkisofs
fingerprint as well as full testing and modifying the inode cache and NFS
export code. For Solaris I am ready now and the code will appear soon in an
official OpenSolaris source......



J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-18 15:18:07

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Bodo Eggert <[email protected]> wrote:

> Luca Tettamanti <[email protected]> wrote:
>
> > +++ b/fs/isofs/rock.c
>
> > +
> > + /* Rock Ridge V1.12, override inode number */
> > + if (rr->len == 44)
> > + inode->i_ino = isonum_733(rr->u.PX.inode);
>
> I think it's wrong again, it will break as soon as rockridge 1.13 defines
> an additional field. s,==,>=, should do the trick.
>
> BTW and without digging in the code: Since version 2 will be binary
> incompatible, is there a version check?

There is de-facto no consistency check on Linux.
It would be easy to create special ISO images that cause Linux to panic from
many other places in the ISO-9660 code.


> BTW2, Just to be cautionous: what will happen if somebody forces the same
> inode number on two different entries?

What happenes when somebody set the traffic light to "green" in all directions?

What happenes if someone sets the same inode number for different files on UDF?

This is something you cannot check.

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-18 22:49:07

by Bodo Eggert

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

On Wed, 18 Oct 2006, Joerg Schilling wrote:
> Bodo Eggert <[email protected]> wrote:

> > BTW2, Just to be cautionous: what will happen if somebody forces the same
> > inode number on two different entries?

[...]
> This is something you cannot check.

Exactly that's why I'd ignore the on-disk "inode number" and instead use
the generated one untill someone comes along with a clever idea to fix
the issue or can show that it's mostly hermless.

--
Artificial Intelligence usually beats real stupidity.

2006-10-18 23:32:39

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Bodo Eggert <[email protected]> wrote:

> On Wed, 18 Oct 2006, Joerg Schilling wrote:
> > Bodo Eggert <[email protected]> wrote:
>
> > > BTW2, Just to be cautionous: what will happen if somebody forces the same
> > > inode number on two different entries?
>
> [...]
> > This is something you cannot check.
>
> Exactly that's why I'd ignore the on-disk "inode number" and instead use
> the generated one untill someone comes along with a clever idea to fix
> the issue or can show that it's mostly hermless.

I could understand you in case that Linux would do some basic consistency checks
in the iso-9660 code already.....

Show me another program besides mkisofs that implements inode numbers _and_ does
it wrong.


J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-19 21:20:24

by Bodo Eggert

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

On Thu, 19 Oct 2006, Joerg Schilling wrote:
> Bodo Eggert <[email protected]> wrote:
> > On Wed, 18 Oct 2006, Joerg Schilling wrote:
> > > Bodo Eggert <[email protected]> wrote:

> > > > BTW2, Just to be cautionous: what will happen if somebody forces the same
> > > > inode number on two different entries?
> >
> > [...]
> > > This is something you cannot check.
> >
> > Exactly that's why I'd ignore the on-disk "inode number" and instead use
> > the generated one untill someone comes along with a clever idea to fix
> > the issue or can show that it's mostly hermless.
>
> I could understand you in case that Linux would do some basic consistency checks
> in the iso-9660 code already.....

ISO9660-over-NFS is no big usecase, linked files on CD aren't common and
the current code just won't benefit from hardlinks. Therefore I don't see
a compelling reason to use this feature without first thinking about
possible attacks.

Even if there are more holes to be plugged, punching even more holes
doesn't help. Instead, we should hope that someone finds the time to
plug these holes and praise him when he comes.

> Show me another program besides mkisofs that implements inode numbers _and_ does
> it wrong.

My hacked mkisofs, which I'll use to r00t you all and gain world domination.
--
"Don't draw fire; it irritates the people around you."
-Your Buddies

2006-10-19 21:32:12

by H. Peter Anvin

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Pekka Enberg wrote:
> On 10/17/06, Luca Tettamanti <[email protected]> wrote:
>> Now, who is in charge of isofs?
>
> Try fsdevel and/or akpm.

I think I might be on the hook for iso9660. Not really sure how that
happened, but people have been sending me patches.

-hpa

2006-10-19 21:42:38

by Joerg.Schilling

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Bodo Eggert <[email protected]> wrote:

> > > Exactly that's why I'd ignore the on-disk "inode number" and instead use
> > > the generated one untill someone comes along with a clever idea to fix
> > > the issue or can show that it's mostly hermless.
> >
> > I could understand you in case that Linux would do some basic consistency checks
> > in the iso-9660 code already.....
>
> ISO9660-over-NFS is no big usecase, linked files on CD aren't common and
> the current code just won't benefit from hardlinks. Therefore I don't see
> a compelling reason to use this feature without first thinking about
> possible attacks.

It looks like you did never really think about the problem or that you just
don't know what you are talking about :-(

A typical UNIX standard installation has more than 3000 hard linked file name
entries in the root filesystem. There are many people who like to backup their
root filesystem via mkisofs and all life CDs and install CDs depend on hard
linked files.

But this seems to be a frequent way of dealing with implementation deficits:
define the problem to be not existant....


> Even if there are more holes to be plugged, punching even more holes
> doesn't help. Instead, we should hope that someone finds the time to
> plug these holes and praise him when he comes.

It seems that you did not think about the problem...

Wrong inode numbes do not open holes, they just create a garbage in garbage out
behavior, but they will not cause the OS to panic.

The unfixed bugs in the Linux iso-9660 implementation on the other side allow
me to create a CD that causes Linux to panic without taking me a long time.

> > Show me another program besides mkisofs that implements inode numbers _and_ does
> > it wrong.
>
> My hacked mkisofs, which I'll use to r00t you all and gain world domination.

This is just a junk statement, describe what you believe than can be done...

J?rg

--
EMail:[email protected] (home) J?rg Schilling D-13353 Berlin
[email protected] (uni)
[email protected] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

2006-10-19 21:46:55

by Alan

[permalink] [raw]
Subject: Re: Linux ISO-9660 Rock Ridge bug needs fix

Ar Iau, 2006-10-19 am 23:39 +0200, ysgrifennodd Joerg Schilling:
> Wrong inode numbes do not open holes, they just create a garbage in garbage out
> behavior, but they will not cause the OS to panic.

Thats implementation dependant as people who stress OS environments and
do fuzz testing have provided again and again and again - including a
previous Linux over ext3 case. There isn't exactly any hurry - its
evident from the lack of reports that you are the first tool author
whose beta tool even uses this feature.

> The unfixed bugs in the Linux iso-9660 implementation on the other side allow
> me to create a CD that causes Linux to panic without taking me a long time.

And those should indeed be fixed.