kernel 2.4.27-10: isofs driver ignore some parameters with mount
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 6 lines · Alexander Pytlev
Hello Debian,
Kernel 2.4.27-10
With mount isofs filesystem, any mount parameters after iocharset=,map=,session= are ignored.
Quoted 8 lines
Sample:
mount -t isofs -o uid=100,iocharset=koi8-r,gid=100
/dev/cdrom/media/cdromgid=100 - was ignored
I look in source and find that problem. I make two patch, simply and full (what addeded some functionality - ignore wrong mount parameters)
Thanks,
I will try and get the simple version of this patch into the next Sarge update.
I have also CCed Marcelo and the LKML for their consideration, as this problem still seems to be present in the lastest 2.4 tree.
Signature · 121 lines
Horms
simply patch:
===================================================================================
--- kernel-source-2.4.27/fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300
+++ kernel-source/fs/isofs/inode.c 2005-08-11 11:55:12.000000000 +0300
@@ -340,13 +340,13 @@
else if (!strcmp(value,"acorn")) popt->map = 'a';
else return 0;
}
- if (!strcmp(this_char,"session") && value) {
+ else if (!strcmp(this_char,"session") && value) {
char * vpnt = value;
unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
if(ivalue < 0 || ivalue >99) return 0;
popt->session=ivalue+1;
}
- if (!strcmp(this_char,"sbsector") && value) {
+ else if (!strcmp(this_char,"sbsector") && value) {
char * vpnt = value;
unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
if(ivalue < 0 || ivalue >660*512) return 0;
===================================================================================
full patch:
===================================================================================
--- kernel-source-2.4.27/fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300
+++ kernel-source/fs/isofs/inode.c 2005-08-11 11:50:56.000000000 +0300
@@ -327,10 +327,11 @@
popt->iocharset = value;
while (*value && *value != ',')
value++;
- if (value == popt->iocharset)
- return 0;
*value = 0;
- } else
+ if (value == popt->iocharset){
+ printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
+ }
+ }
#endif
if (!strcmp(this_char,"map") && value) {
if (value[0] && !value[1] && strchr("ano",*value))
@@ -338,28 +339,30 @@
else if (!strcmp(value,"off")) popt->map = 'o';
else if (!strcmp(value,"normal")) popt->map = 'n';
else if (!strcmp(value,"acorn")) popt->map = 'a';
- else return 0;
+ else printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
}
if (!strcmp(this_char,"session") && value) {
char * vpnt = value;
unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
- if(ivalue < 0 || ivalue >99) return 0;
- popt->session=ivalue+1;
+ if(ivalue < 0 || ivalue >99)
+ printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
+ else popt->session=ivalue+1;
}
if (!strcmp(this_char,"sbsector") && value) {
char * vpnt = value;
unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
- if(ivalue < 0 || ivalue >660*512) return 0;
- popt->sbsector=ivalue;
+ if(ivalue < 0 || ivalue >660*512)
+ printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
+ else popt->sbsector=ivalue;
}
- else if (!strcmp(this_char,"check") && value) {
+ if (!strcmp(this_char,"check") && value) {
if (value[0] && !value[1] && strchr("rs",*value))
popt->check = *value;
else if (!strcmp(value,"relaxed")) popt->check = 'r';
else if (!strcmp(value,"strict")) popt->check = 's';
- else return 0;
+ else printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
}
- else if (!strcmp(this_char,"conv") && value) {
+ if (!strcmp(this_char,"conv") && value) {
/* no conversion is done anymore;
we still accept the same mount options,
but ignore them */
@@ -368,22 +371,24 @@
else if (!strcmp(value,"text")) ;
else if (!strcmp(value,"mtext")) ;
else if (!strcmp(value,"auto")) ;
- else return 0;
+ else printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
}
- else if (value &&
+ if (value &&
(!strcmp(this_char,"block") ||
!strcmp(this_char,"mode") ||
!strcmp(this_char,"uid") ||
!strcmp(this_char,"gid"))) {
char * vpnt = value;
unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
- if (*vpnt) return 0;
+ if (*vpnt) printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
+ else
switch(*this_char) {
case 'b':
if ( ivalue != 512
&& ivalue != 1024
- && ivalue != 2048) return 0;
- popt->blocksize = ivalue;
+ && ivalue != 2048)
+ printk("Invalid or missed parameter:%s=%s,\n",this_char,value);
+ else popt->blocksize = ivalue;
break;
case 'u':
popt->uid = ivalue;
@@ -396,7 +401,6 @@
break;
}
}
- else return 1;
}
return 1;
}
===================================================================================Quoted 16 lines · Horms
Quoted 6 lines · Alexander Pytlev
Hello Debian,
Kernel 2.4.27-10
With mount isofs filesystem, any mount parameters after iocharset=,map=,session= are ignored.Quoted 8 lines
Sample:
mount -t isofs -o uid=100,iocharset=koi8-r,gid=100
/dev/cdrom/media/cdromgid=100 - was ignored
I look in source and find that problem. I make two patch, simply and full (what addeded some functionality - ignore wrong mount parameters)
Quoted 10 lines
Thanks,
I will try and get the simple version of this patch into the next Sarge update.
I have also CCed Marcelo and the LKML for their consideration, as this problem still seems to be present in the lastest 2.4 tree.
Signature · 1 line
Horms
Marcelo and LKML, here is a rediff of the simple version of the patch from Alexander Pytlev that I forwarded previously. The whitespace in his version had been munged.
I haven't tested it, but it looks like it should resolve the problem Alexander reported that mount parameters after iocharset, map and session are ignored.
This should apply against current 2.4 git. I took a peek into 2.6, and the code there has seems to have been completely restructured.
isofs/inode.c renamed from isofs/inode.c.orig +2 -2
@@ -340,13 +340,13 @@else if (!strcmp (value,"acorn" )) popt->map ='a' ;else return 0 ; } -if (!strcmp (this_char,"session" ) && value) { +else if (!strcmp (this_char,"session" ) && value) {char * vpnt = value;unsigned int ivalue =simple_strtoul (vpnt, &vpnt,0 );if (ivalue <0 || ivalue >99 )return 0 ; popt->session=ivalue+1 ; } -if (!strcmp (this_char,"sbsector" ) && value) { +else if (!strcmp (this_char,"sbsector" ) && value) {char * vpnt = value;unsigned int ivalue =simple_strtoul (vpnt, &vpnt,0 );if (ivalue <0 || ivalue >660 *512 )return 0 ;
Hi folks,
Quoted 47 lines · Horms
Quoted 6 lines · Alexander Pytlev
Hello Debian,
Kernel 2.4.27-10
With mount isofs filesystem, any mount parameters after iocharset=,map=,session= are ignored.Quoted 8 lines
Sample:
mount -t isofs -o uid=100,iocharset=koi8-r,gid=100
/dev/cdrom/media/cdromgid=100 - was ignored
I look in source and find that problem. I make two patch, simply and full (what addeded some functionality - ignore wrong mount parameters)
Thanks,
I will try and get the simple version of this patch into the next Sarge update.
I have also CCed Marcelo and the LKML for their consideration, as this problem still seems to be present in the lastest 2.4 tree.
Signature · 4 lines
Horms simply patch: ===================================================================================fs/isofs/inode.c 2005-08-11 11:55:12.000000000 +0300 renamed from fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300 +2
-2@@ -340,13 +340,13 @@ else if (!strcmp(value,"acorn")) popt->map = 'a'; else return 0; } - if (!strcmp(this_char,"session") && value) { + else if (!strcmp(this_char,"session") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >99) return 0; popt->session=ivalue+1; } - if (!strcmp(this_char,"sbsector") && value) { + else if (!strcmp(this_char,"sbsector") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >660*512) return 0;
Quoted 1 line
===================================================================================
Neither "sbsector" or "session" parameters are part of the options string used in Alexander's example, so how come this patch can make any difference?
Usage of "sbsector" or "session" parameters could explain the above patch making a difference because the buggy, always true "(unsigned long) ivalue < 0" comparison invokes "return 0", but that is not the case.
The code after the "popt->iocharset = value;" does not make any sense.
It seems that the "*value = 0" assignment can screw up the rest of the string, isnt that the real issue?
#ifdef CONFIG_JOLIET
if (!strcmp(this_char,"iocharset") && value) {
popt->iocharset = value;
while (*value && *value != ',')
value++;
if (value == popt->iocharset)
return 0;
*value = 0;
} else#endif
Quoted 74 lines · Marcelo Tosatti
Hi folks,
Quoted 47 lines · Horms
Quoted 6 lines · Alexander Pytlev
Hello Debian,
Kernel 2.4.27-10
With mount isofs filesystem, any mount parameters after iocharset=,map=,session= are ignored.Quoted 8 lines
Sample:
mount -t isofs -o uid=100,iocharset=koi8-r,gid=100
/dev/cdrom/media/cdromgid=100 - was ignored
I look in source and find that problem. I make two patch, simply and full (what addeded some functionality - ignore wrong mount parameters)
Thanks,
I will try and get the simple version of this patch into the next Sarge update.
I have also CCed Marcelo and the LKML for their consideration, as this problem still seems to be present in the lastest 2.4 tree.
Signature · 4 lines
Horms simply patch: ===================================================================================fs/isofs/inode.c 2005-08-11 11:55:12.000000000 +0300 renamed from fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300 +2
-2@@ -340,13 +340,13 @@ else if (!strcmp(value,"acorn")) popt->map = 'a'; else return 0; } - if (!strcmp(this_char,"session") && value) { + else if (!strcmp(this_char,"session") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >99) return 0; popt->session=ivalue+1; } - if (!strcmp(this_char,"sbsector") && value) { + else if (!strcmp(this_char,"sbsector") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >660*512) return 0;Quoted 1 line
===================================================================================
Neither "sbsector" or "session" parameters are part of the options string used in Alexander's example, so how come this patch can make any difference?
Usage of "sbsector" or "session" parameters could explain the above patch making a difference because the buggy, always true "(unsigned long) ivalue < 0" comparison invokes "return 0", but that is not the case.
The code after the "
popt->iocharset= value;" does not make any sense.It seems that the "*value = 0" assignment can screw up the rest of the string, isnt that the real issue?
#ifdef CONFIG_JOLIET if (!strcmp(this_char,"iocharset") && value) { popt->iocharset = value; while (*value && *value != ',') value++; if (value == popt->iocharset) return 0; *value = 0; } else
Quoted 1 line
#endif
Sorry about that, while the patch above does seem to be a valid clean up, on further examination I agree that it does not address the problem at hand, and that the problem seems to lie in the *value assignment as you suggest. I wonder if advancing this_char to the character aftter value, if non-NULL would resolve this problem. I'll do some testing, but in the mean time, here is what I have in mind:
fs/isofs/inode.c +4 -1
@@ -329,7 +329,10 @@ value++;if (value == popt->iocharset)return 0 ; - *value =0 ; +if (*value) { + this_char = value +1 ; + *value =0 ; + } } else#endif if (!strcmp (this_char,"map" ) && value) {
Hi,
On Marcelo's request I have taken a closer look at this. It seems that Alexander Pytlev's original (simple) patch was correct.
Without it the logic looks a bit like this.
while (...) {
if iocharset
...
else if map
...
if session
...
if sbsector
...
else if check
...
...
else
return 1;
}Now, if iocharset, map or session are matched, then none of the if or else if clauses under sbsector will match (that is none of these clauses match iocharset, map or session), and thus the else clause will be hit, and the function will return 1 without parsing any furhter options.
With Alexander's fix, the if session and if sbsector clauses become else if, and its easy to see that the return 1 won't be premeturely called.
I have tested that this patch works using the testcase options iocharset=koi8-r,gid=100, and checking that gid is set correctly with the patch, and incorrectly without.
Here is the patch and signoff again, just for the record. I will send a second patch to clean up the *value = 0 code that Marcelo cast concerns over - its bogus but harmless.
fs/isofs/inode.c +2 -2
@@ -340,13 +337,13 @@else if (!strcmp (value,"acorn" )) popt->map ='a' ;else return 0 ; } -if (!strcmp (this_char,"session" ) && value) { +else if (!strcmp (this_char,"session" ) && value) {char * vpnt = value;unsigned int ivalue =simple_strtoul (vpnt, &vpnt,0 );if (ivalue <0 || ivalue >99 )return 0 ; popt->session=ivalue+1 ; } -if (!strcmp (this_char,"sbsector" ) && value) { +else if (!strcmp (this_char,"sbsector" ) && value) {char * vpnt = value;unsigned int ivalue =simple_strtoul (vpnt, &vpnt,0 );if (ivalue <0 || ivalue >660 *512 )return 0 ;
PATCH 1/1 Bogus code in parsing of iocharset in isofs
Hi,
this is a followup for the patch I sent earlier (like about 2 minutes ago) regarding isofs options parsing. In the course of debuging this Marcelo pointed out the following code
#ifdef CONFIG_JOLIET
if (!strcmp(this_char,"iocharset") && value) {
popt->iocharset = value;
while (*value && *value != ',')
value++;
if (value == popt->iocharset)
return 0;
*value = 0;
} else #endif
On inspection it turns out that because of use of strtok(), *value is already NULL terminated, and thus the code snippet above is largely bogus. The following patch should remove the bogus code without changing functionality.
isofs/inode.c renamed from build-386/fs/isofs/inode.c +2 -5
@@ -324,12 +324,9 @@#ifdef CONFIG_JOLIET if (!strcmp (this_char,"iocharset" ) && value) { - popt->iocharset = value; -while (*value && *value !=',' ) - value++; -if (value == popt->iocharset) +if (!value)return 0 ; - *value =0 ; + popt->iocharset = value; } else#endif if (!strcmp (this_char,"map" ) && value) {