| Message ID | 200907210058.44737.mb@bu3sch.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 07/21/2009 12:58 AM, Michael Buesch wrote: > loff_t is a signed type. If userspace passes a negative ppos, the "count" > range check is weakened. "count"s bigger than HPEE_MAX_LENGTH will pass the check. > Also, if ppos is negative, the readb(eisa_eeprom_addr + *ppos) will poke in random > memory. > > Signed-off-by: Michael Buesch<mb@bu3sch.de> > Cc: stable@kernel.org Thanks! Applied and pushed upstream. Helge > Patch is untested due to lack of hardware. > > --- > drivers/parisc/eisa_eeprom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- linux-2.6.orig/drivers/parisc/eisa_eeprom.c > +++ linux-2.6/drivers/parisc/eisa_eeprom.c > @@ -48,21 +48,21 @@ static loff_t eisa_eeprom_llseek(struct > return (offset>= 0&& offset< HPEE_MAX_LENGTH) ? (file->f_pos = offset) : -EINVAL; > } > > static ssize_t eisa_eeprom_read(struct file * file, > char __user *buf, size_t count, loff_t *ppos ) > { > unsigned char *tmp; > ssize_t ret; > int i; > > - if (*ppos>= HPEE_MAX_LENGTH) > + if (*ppos< 0 || *ppos>= HPEE_MAX_LENGTH) > return 0; > > count = *ppos + count< HPEE_MAX_LENGTH ? count : HPEE_MAX_LENGTH - *ppos; > tmp = kmalloc(count, GFP_KERNEL); > if (tmp) { > for (i = 0; i< count; i++) > tmp[i] = readb(eisa_eeprom_addr+(*ppos)++); > > if (copy_to_user (buf, tmp, count)) > ret = -EFAULT; > -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-08-05 at 20:38 +0200, Helge Deller wrote: > On 07/21/2009 12:58 AM, Michael Buesch wrote: > > loff_t is a signed type. If userspace passes a negative ppos, the "count" > > range check is weakened. "count"s bigger than HPEE_MAX_LENGTH will pass the check. > > Also, if ppos is negative, the readb(eisa_eeprom_addr + *ppos) will poke in random > > memory. > > > > Signed-off-by: Michael Buesch<mb@bu3sch.de> > > Cc: stable@kernel.org > > Thanks! > > Applied and pushed upstream. Hang on a minute, this is an untested patch. True, it will likely cause no harm, but it would be more usual to wait for the actual confirmation before declaring the problem fixed. I'm also very concerned about this: http://lkml.org/lkml/2009/8/2/107 That's a breach of standard maintainer protocol since you failed to copy the architecture list on the pull request. Parisc is in a precarious position as a marginal architecture that isn't being produced any more. Having duelling trees and maintainers is definitely very unhelpful because it could cause Linus to lose confidence in our ability as a community. First things first, you need to agree on a single tree ... although it's perfectly possible to have multiple maintainers commit to it (x86 works this way), can we do this at least before the schizophrenia gets noticed? Thanks, James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 05, 2009 at 02:57:01PM -0500, James Bottomley wrote: > Hang on a minute, this is an untested patch. True, it will likely cause > no harm, but it would be more usual to wait for the actual confirmation > before declaring the problem fixed. > I'd be shocked if anyone was actually in a position to test this at all... I don't remember where I put my last Mongoose and HP VG ethernet card... It looked fine to me though. > I'm also very concerned about this: > > http://lkml.org/lkml/2009/8/2/107 > > That's a breach of standard maintainer protocol since you failed to copy > the architecture list on the pull request. > > Parisc is in a precarious position as a marginal architecture that isn't > being produced any more. Having duelling trees and maintainers is > definitely very unhelpful because it could cause Linus to lose > confidence in our ability as a community. > > First things first, you need to agree on a single tree ... although it's > perfectly possible to have multiple maintainers commit to it (x86 works > this way), can we do this at least before the schizophrenia gets > noticed? > I think Helge was just upset that I wasn't merging things fast enough, and, fair enough, I guess. I promise to rectify that, and if I don't, I plan to step aside. regards, Kyle -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 05, 2009 at 04:14:56PM -0400, Kyle McMartin wrote: > On Wed, Aug 05, 2009 at 02:57:01PM -0500, James Bottomley wrote: > > Hang on a minute, this is an untested patch. True, it will likely cause > > no harm, but it would be more usual to wait for the actual confirmation > > before declaring the problem fixed. > > > > I'd be shocked if anyone was actually in a position to test this at > all... I don't remember where I put my last Mongoose and HP VG ethernet > card... It looked fine to me though. My 725 is sitting idle in the rack ... I think it's got two EISA cards in it. I've also got a Mongoose card for the 715. There's two problems that would prevent me from testing this. One is that I don't have any HIL devices, and the current HIL driver goes insane with printks if you have no HIL deviecs plugged in. The other is that the EISA code refuses to work until the EISA cards have been configured, and we don't have a Linux utility to configure EISA cards.
On Wed, Aug 05, 2009 at 02:20:57PM -0600, Matthew Wilcox wrote: > On Wed, Aug 05, 2009 at 04:14:56PM -0400, Kyle McMartin wrote: > > On Wed, Aug 05, 2009 at 02:57:01PM -0500, James Bottomley wrote: > > > Hang on a minute, this is an untested patch. True, it will likely cause > > > no harm, but it would be more usual to wait for the actual confirmation > > > before declaring the problem fixed. > > > > > > > I'd be shocked if anyone was actually in a position to test this at > > all... I don't remember where I put my last Mongoose and HP VG ethernet > > card... It looked fine to me though. > > My 725 is sitting idle in the rack ... I think it's got two EISA cards > in it. I've also got a Mongoose card for the 715. > > There's two problems that would prevent me from testing this. > > One is that I don't have any HIL devices, and the current HIL driver > goes insane with printks if you have no HIL deviecs plugged in. > > The other is that the EISA code refuses to work until the EISA cards > have been configured, and we don't have a Linux utility to configure > EISA cards. > It just so happens I have a PS2<->HIL thingy doodad around here. :) -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/05/2009 10:14 PM, Kyle McMartin wrote: > On Wed, Aug 05, 2009 at 02:57:01PM -0500, James Bottomley wrote: >> I'm also very concerned about this: >> >> http://lkml.org/lkml/2009/8/2/107 >> >> That's a breach of standard maintainer protocol since you failed to copy >> the architecture list on the pull request. Yes, I sadly missed to copy parisc-mailing-list, but at least I remembered to copy Kyle. >> Parisc is in a precarious position as a marginal architecture that isn't >> being produced any more. Having duelling trees and maintainers is >> definitely very unhelpful because it could cause Linus to lose >> confidence in our ability as a community. Agreed. >> First things first, you need to agree on a single tree ... although it's >> perfectly possible to have multiple maintainers commit to it (x86 works >> this way), can we do this at least before the schizophrenia gets >> noticed? Yep. > I think Helge was just upset that I wasn't merging things fast enough, > and, fair enough, I guess. I promise to rectify that, and if I don't, I > plan to step aside. Thanks Kyle! Yes, this was the only reason. Just a few word on this whole thread, and the unplanned discussion on lkml... When I sent the pull request, my intention was never to offend Kyle or any other parisc developer. I was even astonished that Kyle offended me suddenly that harsh in public. Neither was my intend to create a duelling tree to the one from Kyle. I just wanted to make sure, that the latest important patches (e.g. the GOT fix for 64bit kernel modules) just would not miss the 2.6.31 kernel. As we all know, Debian developers lately discussed regularily, if the parisc port should be dropped as a stable/release platform. IMHO, one of the main reasons for the bad state is, that often patches were not merged upstream (or at least not in time), and then backporting them was complicated and often missed. My pull request included only simple patches. It was planned as a one-time thing just to help Kyle with the maintenance of the outstanding patches. I agree with Kyle, that I should have sent him a notice _before_ sending the pull-request to lkml. Again, I just didn't thought he would be offended by this, esp. since it was just a collection of patches which went to the parisc-list, with a few simple patches by me. Regarding the push of outstanding patches, I'd really very much prefer a joint tree, to which the maintainers can push. That way everyone can regularily pull a working parisc tree. Furthermore, I'd prefer if we can get the timing right, that most of the outstanding patches goes upstream with a -rc1 release, and then only a minor patchset would be pushed in -rc5 or similar time frame. Helge PS: James, welcome as a parisc maintainer! (http://git.kernel.org/?p=linux/kernel/git/kyle/parisc-2.6.git;a=commitdiff;h=4b5273681a72dbf5ece64d0ae1e85d54722012fe) -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-2.6.orig/drivers/parisc/eisa_eeprom.c +++ linux-2.6/drivers/parisc/eisa_eeprom.c @@ -48,21 +48,21 @@ static loff_t eisa_eeprom_llseek(struct return (offset >= 0 && offset < HPEE_MAX_LENGTH) ? (file->f_pos = offset) : -EINVAL; } static ssize_t eisa_eeprom_read(struct file * file, char __user *buf, size_t count, loff_t *ppos ) { unsigned char *tmp; ssize_t ret; int i; - if (*ppos >= HPEE_MAX_LENGTH) + if (*ppos < 0 || *ppos >= HPEE_MAX_LENGTH) return 0; count = *ppos + count < HPEE_MAX_LENGTH ? count : HPEE_MAX_LENGTH - *ppos; tmp = kmalloc(count, GFP_KERNEL); if (tmp) { for (i = 0; i < count; i++) tmp[i] = readb(eisa_eeprom_addr+(*ppos)++); if (copy_to_user (buf, tmp, count)) ret = -EFAULT;
loff_t is a signed type. If userspace passes a negative ppos, the "count" range check is weakened. "count"s bigger than HPEE_MAX_LENGTH will pass the check. Also, if ppos is negative, the readb(eisa_eeprom_addr + *ppos) will poke in random memory. Signed-off-by: Michael Buesch <mb@bu3sch.de> Cc: stable@kernel.org --- Patch is untested due to lack of hardware. --- drivers/parisc/eisa_eeprom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)