| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 23 May 2009 15:00:37 +0200
From: Karsten Keil <keil@...systems.de>
To: Sam Ravnborg <sam@...nborg.org>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
David Miller <davem@...emloft.net>,
i4ldeveloper@...tserv.isdn4linux.de
Subject: Re: [mISDN PATCH v2 09/19] Fix TEI and SAPI handling
Hi Sam,
thanks for the review.
On Freitag, 22. Mai 2009 23:53:14 Sam Ravnborg wrote:
> Hi Karsten.
>
> Flash from the past looking at some ISDN layer2.
> Some comments below.
>
> Sam
>
> > struct layer2 *
> > -create_l2(struct mISDNchannel *ch, u_int protocol, u_long options,
> > u_long arg) +create_l2(struct mISDNchannel *ch, u_int protocol, u_long
> > options, u_int tei, + u_int sapi)
>
> Unrealted to this specific patch...
> Are there any specific reason why mISDN prefer bsd style (u_int)
> rather then linux style (u32)?
No, this was some historic process with the ISDN code.
I think, if we want change this we should make a separate patch to change
this in all mISDN code later, I usually prefer shorter type names as well :-).
So as rule of dumb:
- Use uN/sN for all values with constant width N ?
What should be used on functions which are usually defined as int and
only return 0 or some negative ERROR code, I think int should be OK here ?
>
> > static struct layer2 *
> > -create_new_tei(struct manager *mgr, int tei)
> > +create_new_tei(struct manager *mgr, int tei, int sapi)
> > {
>
> Here tei and sapi are passed as signed.
> But valid sapi range is [0..63] and tei [0..127].
> And create_l2 above uses unsigned.
>
> This looks confusing.
I think the signed was selected for error handling, but you are correct then
it should be the same in all functions.
>
> > u_long opt = 0;
> > u_long flags;
> > @@ -791,7 +791,7 @@ create_new_tei(struct manager *mgr, int tei)
> > if (mgr->ch.st->dev->Dprotocols
> > & ((1 << ISDN_P_TE_E1) | (1 << ISDN_P_NT_E1)))
> > test_and_set_bit(OPTION_L2_PMX, &opt);
> > - l2 = create_l2(mgr->up, ISDN_P_LAPD_NT, (u_int)opt, (u_long)tei);
> > + l2 = create_l2(mgr->up, ISDN_P_LAPD_NT, (u_int)opt, tei, sapi);
> > if (!l2) {
> > printk(KERN_WARNING "%s:no memory for layer2\n", __func__);
> > return NULL;
> > @@ -839,12 +839,15 @@ new_tei_req(struct manager *mgr, u_char *dp)
> > ri += dp[1];
> > if (!mgr->up)
> > goto denied;
> > - tei = get_free_tei(mgr);
> > + if (dp[3] != 0xff)
> > + tei = dp[3] >> 1; /* 3GPP TS 08.56 6.1.11.2 */
> > + else
> > + tei = get_free_tei(mgr);
>
> You should check EA0 here before assuming that any values except
> EA=1, tei=127 equals ptmp.
>
make sense.
> > if (tei < 0) {
> > printk(KERN_WARNING "%s:No free tei\n", __func__);
> > goto denied;
> > }
>
> So get_free_tei() may return a negative value indicating no free tei.
> So that make my comment above void - but is this really the best way
> to return an error. Possibly it is..
>
We could define 255 as the error value, I will think about this, but in
general I prefer negative values for error cases.
> > - l2 = create_new_tei(mgr, tei);
> > + l2 = create_new_tei(mgr, tei, 0);
>
> In general I would prefer to read: SAPI_CALLCONTROL rahter than a hardcoded
> 0.
>
Yes.
> > if (!l2)
> > goto denied;
> > else
> > @@ -976,8 +979,6 @@ create_teimgr(struct manager *mgr, struct channel_req
> > *crq) __func__, dev_name(&mgr->ch.st->dev->dev),
> > crq->protocol, crq->adr.dev, crq->adr.channel,
> > crq->adr.sapi, crq->adr.tei);
> > - if (crq->adr.sapi != 0) /* not supported yet */
> > - return -EINVAL;
> > if (crq->adr.tei > GROUP_TEI)
> > return -EINVAL;
> > if (crq->adr.tei < 64)
> > @@ -1025,7 +1026,7 @@ create_teimgr(struct manager *mgr, struct
> > channel_req *crq) return 0;
> > }
> > l2 = create_l2(crq->ch, crq->protocol, (u_int)opt,
> > - (u_long)crq->adr.tei);
> > + crq->adr.tei, crq->adr.sapi);
> > if (!l2)
> > return -ENOMEM;
> > l2->tm = kzalloc(sizeof(struct teimgr), GFP_KERNEL);
> > @@ -1166,7 +1167,7 @@ static int
> > check_data(struct manager *mgr, struct sk_buff *skb)
> > {
> > struct mISDNhead *hh = mISDN_HEAD_P(skb);
> > - int ret, tei;
> > + int ret, tei, sapi;
> > struct layer2 *l2;
> >
> > if (*debug & DEBUG_L2_CTRL)
> > @@ -1178,18 +1179,16 @@ check_data(struct manager *mgr, struct sk_buff
> > *skb) return -ENOTCONN;
> > if (skb->len != 3)
> > return -ENOTCONN;
> > - if (skb->data[0] != 0)
> > - /* only SAPI 0 command */
> > - return -ENOTCONN;
> > + sapi = skb->data[0] >> 2;
>
> PReviously there was an implicit check of EA0.
> This is missed not that you read sapi and discard the two lower bits.
>
> I also wonder if you remeber to check the command/response bit.
> That was implicitly tested to be 0 before and also ignored now.
Yes, you are correct.
>
> > if (!(skb->data[1] & 1)) /* invalid EA1 */
> > return -EINVAL;
> > - tei = skb->data[1] >> 0;
> > + tei = skb->data[1] >> 1;
>
> This looks like a bug-fix...
>
Yes, I think fixed TEI was never tested with P2MP, I know
no device which really use it with SAPI 0, only in P2P we use a
fixed TEI of 0. Also the test suite used by certifications did
not check for correct fixed TEI handling, it only checks that a
fixed TEI is rejected in a dynamic TEI assignment.
With SAPI != 0 fixed TEIs are more common, so the bug was
detected now.
> > if (tei > 63) /* not a fixed tei */
> > return -ENOTCONN;
> > if ((skb->data[2] & ~0x10) != SABME)
> > return -ENOTCONN;
> > /* We got a SABME for a fixed TEI */
> > - l2 = create_new_tei(mgr, tei);
> > + l2 = create_new_tei(mgr, tei, sapi);
> > if (!l2)
> > return -ENOMEM;
> > ret = l2->ch.send(&l2->ch, skb);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists