isdn: pcbit: another off-by-one issue?

isdn: pcbit: another off-by-one issue?

[Rasmus Villemoes]

Hi Dan

You were last to touch drivers/isdn/pcbit/drv.c (7bcc6738eef), but I
think there may still be an off-by-one in pcbit_set_msn: At the end of
the loop, sp is incremented by len, but if the string contained a comma,
sp will now point at that. At that point, we seem to be stuck in an
infinite loop where we'll always get cp==sp and len==0, until we run out
of memory.

Am I reading this completely wrong?

Rasmus

Re: isdn: pcbit: another off-by-one issue?

[Dan Carpenter]

On Wed, Jun 10, 2015 at 09:50:53PM +0200, Rasmus Villemoes wrote:
> Hi Dan
> 
> You were last to touch drivers/isdn/pcbit/drv.c (7bcc6738eef), but I
> think there may still be an off-by-one in pcbit_set_msn: At the end of
> the loop, sp is incremented by len, but if the string contained a comma,
> sp will now point at that. At that point, we seem to be stuck in an
> infinite loop where we'll always get cp==sp and len==0, until we run out
> of memory.
> 
> Am I reading this completely wrong?

Nope.  You're right.  That bug has been there since before the start of
git.  We could fix it by doing:

diff --git a/drivers/isdn/pcbit/drv.c b/drivers/isdn/pcbit/drv.c
index 4172e22..b156d5b 100644
--- a/drivers/isdn/pcbit/drv.c
+++ b/drivers/isdn/pcbit/drv.c
@@ -1053,7 +1053,7 @@ static void pcbit_set_msn(struct pcbit_dev *dev, char *list)
 		else
 			back->next = ptr;
 		back = ptr;
-		sp += len;
+		sp += len + 1;
 	} while (cp);
 }
 

regards,
dan carpenter

Re: isdn: pcbit: another off-by-one issue?

[Rasmus Villemoes]

[adding some emails I should Cc'ed in the first place]

On Thu, Jun 11 2015, Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Jun 10, 2015 at 09:50:53PM +0200, Rasmus Villemoes wrote:
>> Hi Dan
>> 
>> You were last to touch drivers/isdn/pcbit/drv.c (7bcc6738eef), but I
>> think there may still be an off-by-one in pcbit_set_msn: At the end of
>> the loop, sp is incremented by len, but if the string contained a comma,
>> sp will now point at that. At that point, we seem to be stuck in an
>> infinite loop where we'll always get cp==sp and len==0, until we run out
>> of memory.
>> 
>> Am I reading this completely wrong?
>
> Nope.  You're right.  That bug has been there since before the start of
> git.  We could fix it by doing:
>
> diff --git a/drivers/isdn/pcbit/drv.c b/drivers/isdn/pcbit/drv.c
> index 4172e22..b156d5b 100644
> --- a/drivers/isdn/pcbit/drv.c
> +++ b/drivers/isdn/pcbit/drv.c
> @@ -1053,7 +1053,7 @@ static void pcbit_set_msn(struct pcbit_dev *dev, char *list)
>  		else
>  			back->next = ptr;
>  		back = ptr;
> -		sp += len;
> +		sp += len + 1;
>  	} while (cp);
>  }

Yep, that's also what I would do. 

Since nobody seems to have been hit by this ever, I wonder whether it's
stable@ material. It probably doesn't make sense to fix this without
also backporting 7bcc6738eef.

Rasmus

Re: isdn: pcbit: another off-by-one issue?

[Paul Bolle]

On Thu, 2015-06-11 at 11:28 +0200, Rasmus Villemoes wrote:
> Since nobody seems to have been hit by this ever, I wonder whether it's
> stable@ material. It probably doesn't make sense to fix this without
> also backporting 7bcc6738eef.

I'm guessing nobody is using this anymore. I would be really surprised
if anyone is. Let's test my idea. Hands up anyone that has:
- a PCBIT ISDN-card ("manufactured in Portugal by Octal", according to
  its Kconfig entry);
- a working X86 machine with ISA;
- a working ISDN line;
- and, say, an ISP picking up the stuff you send down that ISDN line
  at the other end;
- and cares how 2.6.32 (and higher) runs that setup.

[Crickets.]

Besides, it is, apparently, an I4L driver. (Though there's a CAPI part
too. I need to check how that fits into the picture.) I4L is deprecated
for ten years now (or is it even longer?).

Perhaps pcbit, and the other I4L drivers, should be (finally) tossed
out? That was discussed last year too, but nothing really was decided.


Paul Bolle

Re: isdn: pcbit: another off-by-one issue?

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

Am 11.06.2015 um 13:44 schrieb Paul Bolle:
> Besides, it is, apparently, an I4L driver. (Though there's a CAPI part
> too. I need to check how that fits into the picture.)

If I read the code correctly it talks I4L to the kernel but CAPI to the
card. This would have been a natural candidate for porting to the kernel
CAPI interface, had anyone bothered. Which reinforces the impression
that no-one is using this anymore.

Note also that the FTP URL mentioned in Documentation/isdn/README.pcbit
doesn't exist anymore.

-- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]