Linux ISO-9660 Rock Ridge bug needs fix

Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Ismail Donmez]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

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

[-- Attachment #2: rock.patch --]
[-- Type: text/x-diff, Size: 644 bytes --]

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;
 		case SIG('P', 'N'):
 			{
diff --git a/fs/isofs/rock.h b/fs/isofs/rock.h
index ed09e2b..faf92c6 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 ino[8];
 };
 
 struct RR_PN_s {

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Luca Tettamanti]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF8, Size: 2239 bytes --]

Ismail Donmez <ismail@pardus.org.tr> 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 <kronos.it@gmail.com>

---
 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.

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Ismail Donmez]

17 Eki 2006 Sal 21:02 tarihinde, Luca Tettamanti şunları yazmıştı: 
> Ismail Donmez <ismail@pardus.org.tr> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Ismail Donmez <ismail@pardus.org.tr> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Ismail Donmez]

17 Eki 2006 Sal 21:16 tarihinde, Joerg Schilling şunları yazmıştı: 
> Ismail Donmez <ismail@pardus.org.tr> 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)

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Luca Tettamanti]

Il Tue, Oct 17, 2006 at 08:16:07PM +0200, Joerg Schilling ha scritto: 
> Ismail Donmez <ismail@pardus.org.tr> 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;}

Re: Re: Linux ISO-9660 Rock Ridge bug needs fix

[Pekka Enberg]

On 10/17/06, Luca Tettamanti <kronos.it@gmail.com> wrote:
> Now, who is in charge of isofs?

Try fsdevel and/or akpm.

                             Pekka

Re: Linux ISO-9660 Rock Ridge bug needs fix

[H. Peter Anvin]

Pekka Enberg wrote:
> On 10/17/06, Luca Tettamanti <kronos.it@gmail.com> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Ismail Donmez <ismail@pardus.org.tr> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Ismail Donmez]

17 Eki 2006 Sal 21:12 tarihinde, Joerg Schilling şunları yazmıştı: 
> Ismail Donmez <ismail@pardus.org.tr> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Ismail Donmez <ismail@pardus.org.tr> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Ismail Donmez]

17 Eki 2006 Sal 21:32 tarihinde, Joerg Schilling şunları yazmıştı: 
> Ismail Donmez <ismail@pardus.org.tr> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[James Lamanna]

Joerg Shilling wrote:
> Ismail Donmez <ismail@pardus.org.tr> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

James Lamanna <jlamanna@gmail.com> wrote:

>
> Joerg Shilling wrote:
> > Ismail Donmez <ismail@pardus.org.tr> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[James Lamanna]

On 10/17/06, Joerg Schilling <Joerg.Schilling@fokus.fraunhofer.de> wrote:
> James Lamanna <jlamanna@gmail.com> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

"James Lamanna" <jlamanna@gmail.com> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Alan Cox]

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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Joerg.Schilling@fokus.fraunhofer.de (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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Jan Engelhardt]

>> > 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'
-- 

Re: Linux ISO-9660 Rock Ridge bug needs fix

[James Lamanna]

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 <jlamanna@gmail.com>
---
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
+ *  (jlamanna@gmail.com)       : 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;

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Bodo Eggert]

Luca Tettamanti <kronos.it@gmail.com> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Bodo Eggert <7eggert@elstempel.de> wrote:

> Luca Tettamanti <kronos.it@gmail.com> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Bodo Eggert]

On Wed, 18 Oct 2006, Joerg Schilling wrote:
> Bodo Eggert <7eggert@elstempel.de> 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. 

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Bodo Eggert <7eggert@gmx.de> wrote:

> On Wed, 18 Oct 2006, Joerg Schilling wrote:
> > Bodo Eggert <7eggert@elstempel.de> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Bodo Eggert]

On Thu, 19 Oct 2006, Joerg Schilling wrote:
> Bodo Eggert <7eggert@gmx.de> wrote:
> > On Wed, 18 Oct 2006, Joerg Schilling wrote:
> > > Bodo Eggert <7eggert@elstempel.de> 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

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Joerg Schilling]

Bodo Eggert <7eggert@gmx.de> 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:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js@cs.tu-berlin.de                (uni)  
       schilling@fokus.fraunhofer.de     (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily

Re: Linux ISO-9660 Rock Ridge bug needs fix

[Alan Cox]

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.