[PATCH] parisc-isa-eeprom: Fix loff_t usage

[PATCH] parisc-isa-eeprom: Fix loff_t usage

[Michael Buesch]

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(-)

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

-- 
Greetings, Michael.

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[Helge Deller]

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

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[James Bottomley]

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

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[Kyle McMartin]

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

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[Matthew Wilcox]

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.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[Kyle McMartin]

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.

:)

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[John David Anglin]

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

I agree with James.  Kyle, your support is crucial, but you don't have
to carry the full burden.  A single tree that is updated regularly makes
it easy for all maintainers to commit to, and for everone to test the
changes in the tree that haven't been pushed upstream.

I'm also pushing for stable parisc trees as well if there are enough relevant
changes to make this useful.

I know GCC's procedure are different, but it has a single master tree
with several hundred maintainers.  A maintainer has either global commit,
partial commit, or commit after approval priviledge.  There is a
test requirement for all changes, and all changes have to been sent to
the relevant lists.  Sometimes things break as a result of a change,
but not that often.

I think all the parisc maintainers should be able to push changes
upstream, but the list should be notified when this is done.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: [PATCH] parisc-isa-eeprom: Fix loff_t usage

[Helge Deller]

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)