Message ID | 20110331180225.GA6677@midget.suse.cz |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Mar 31, 2011 at 08:02:25PM +0200, Jiri Bohac wrote: > However, I wonder how much sense it makes to continue parsing the > facilities if an unknown facility family appears. We don't know > the length of its data, so we will interpret each 16 bytes a new oops, typo: s/16 bytes a new/16 bits as a new/ > facilities header, hopefully soon bailing out on *p != 0x00. > > In case of a long packet where every other byte is zero, the loop > will spam the kernel log with the printk ... which could probably > be classified as a security problem on its own. So how about the > following instead? I have no idea if this breaks some rose > specification, though.
From: Jiri Bohac <jbohac@suse.cz> Date: Thu, 31 Mar 2011 20:02:25 +0200 > So, to get the old behaviour back: Jiri, please do not submit two patches in one email, it's beyond confusing. Instead, please submit a proper two-patch series. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-03-31 at 20:02 +0200, Jiri Bohac wrote: [...] > This last hunk does not look correct. In the default branch of > the switch, you set len = 1, which means > p += 2; facilities_len -= 2. > > The original code does > facilities_len--; p++; > ... and it looks correct. So, to get the old behaviour back: > > diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c > index f6c71ca..9777700 100644 > --- a/net/rose/rose_subr.c > +++ b/net/rose/rose_subr.c > @@ -418,7 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len, > > default: > printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p); > - len = 1; > + len = 0; > break; > } Yes, agreed. > However, I wonder how much sense it makes to continue parsing the > facilities if an unknown facility family appears. We don't know > the length of its data, so we will interpret each 16 bytes a new > facilities header, hopefully soon bailing out on *p != 0x00. > > In case of a long packet where every other byte is zero, the loop > will spam the kernel log with the printk ... which could probably > be classified as a security problem on its own. So how about the > following instead? I have no idea if this breaks some rose > specification, though. [...] I don't know any more than you do; maybe Ralf knows or can find out. Ben.
On Fri, Apr 01, 2011 at 09:41:48PM -0700, David Miller wrote: > From: Jiri Bohac <jbohac@suse.cz> > Date: Thu, 31 Mar 2011 20:02:25 +0200 > > So, to get the old behaviour back: > > Jiri, please do not submit two patches in one email, it's beyond > confusing. > > Instead, please submit a proper two-patch series. The two one line patches were two suggestions what to do. It's either one or the other... Sorry for the confusion.
diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c index f6c71ca..9777700 100644 --- a/net/rose/rose_subr.c +++ b/net/rose/rose_subr.c @@ -418,7 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len, default: printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p); - len = 1; + len = 0; break; } However, I wonder how much sense it makes to continue parsing the facilities if an unknown facility family appears. We don't know the length of its data, so we will interpret each 16 bytes a new facilities header, hopefully soon bailing out on *p != 0x00. In case of a long packet where every other byte is zero, the loop will spam the kernel log with the printk ... which could probably be classified as a security problem on its own. So how about the following instead? I have no idea if this breaks some rose specification, though. Signed-off-by: Jiri Bohac <jbohac@suse.cz> diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c index f6c71ca..e687c7f 100644 --- a/net/rose/rose_subr.c +++ b/net/rose/rose_subr.c @@ -418,8 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len, default: printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p); - len = 1; - break; + return 0; } if (len < 0)