2015-06-10 19:51:07

by Rasmus Villemoes

[permalink] [raw]
Subject: isdn: pcbit: another off-by-one issue?

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


2015-06-11 07:59:06

by Dan Carpenter

[permalink] [raw]
Subject: Re: isdn: pcbit: another off-by-one issue?

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

2015-06-11 09:28:28

by Rasmus Villemoes

[permalink] [raw]
Subject: Re: isdn: pcbit: another off-by-one issue?

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

On Thu, Jun 11 2015, Dan Carpenter <[email protected]> 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

2015-06-11 11:44:33

by Paul Bolle

[permalink] [raw]
Subject: Re: isdn: pcbit: another off-by-one issue?

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

2015-06-30 21:46:28

by Tilman Schmidt

[permalink] [raw]
Subject: Re: isdn: pcbit: another off-by-one issue?

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: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


Attachments:
signature.asc (473.00 B)
OpenPGP digital signature