parisc-isa-eeprom: Fix loff_t usage
diff mbox

Message ID 200907210058.44737.mb@bu3sch.de
State Accepted
Headers show

Commit Message

Michael Buesch July 20, 2009, 10:58 p.m. UTC
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(-)

Comments

Helge Deller Aug. 5, 2009, 6:38 p.m. UTC | #1
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
James Bottomley Aug. 5, 2009, 7:57 p.m. UTC | #2
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
kyle mcmartin Aug. 5, 2009, 8:14 p.m. UTC | #3
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
Matthew Wilcox Aug. 5, 2009, 8:20 p.m. UTC | #4
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.
kyle mcmartin Aug. 5, 2009, 8:37 p.m. UTC | #5
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
Helge Deller Aug. 5, 2009, 10:21 p.m. UTC | #6
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

Patch
diff mbox

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