From 1bdc07ebabefd19b56d1d36584a401ff6085fa71 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:29 +0200 Subject: [PATCH 01/12] isdn/gigaset: missing break in do_facility_req If we take the unsupported supplementary service notification mask path, we end up falling through and overwriting the error code. Insert a break statement to skip the remainder of the switch case and proceed to sending the reply message. Spotted with Coverity. Reported-by: Dave Jones Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/gigaset/capi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c index 3286903a95d2..a2eabe97deea 100644 --- a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@ -1180,6 +1180,7 @@ static void do_facility_req(struct gigaset_capi_ctr *iif, confparam[3] = 2; /* length */ capimsg_setu16(confparam, 4, CapiSupplementaryServiceNotSupported); + break; } info = CapiSuccess; confparam[3] = 2; /* length */ From ee7ff5fed25711a26da7826071e6ede8af049ad2 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:29 +0200 Subject: [PATCH 02/12] isdn/gigaset: make sure controller name is null terminated In gigaset_isdn_regdev, the name field may not have a null terminator if the source string's length is equal to the buffer size. Fix by zero filling the structure and excluding the last byte of the name field from the copy. Spotted with Coverity. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/gigaset/capi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c index a2eabe97deea..044392cba262 100644 --- a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@ -2358,7 +2358,7 @@ int gigaset_isdn_regdev(struct cardstate *cs, const char *isdnid) struct gigaset_capi_ctr *iif; int rc; - iif = kmalloc(sizeof(*iif), GFP_KERNEL); + iif = kzalloc(sizeof(*iif), GFP_KERNEL); if (!iif) { pr_err("%s: out of memory\n", __func__); return -ENOMEM; @@ -2367,7 +2367,7 @@ int gigaset_isdn_regdev(struct cardstate *cs, const char *isdnid) /* prepare controller structure */ iif->ctr.owner = THIS_MODULE; iif->ctr.driverdata = cs; - strncpy(iif->ctr.name, isdnid, sizeof(iif->ctr.name)); + strncpy(iif->ctr.name, isdnid, sizeof(iif->ctr.name) - 1); iif->ctr.driver_name = "gigaset"; iif->ctr.load_firmware = NULL; iif->ctr.reset_ctr = NULL; From 097933ddcd28ef99c116651b20fd2e06717e0f0d Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:29 +0200 Subject: [PATCH 03/12] isdn/gigaset: limit raw CAPI message dump length In dump_rawmsg, the length field from a received data package was used unscrutinized, allowing an attacker to control the size of the allocated buffer and the number of times the output loop iterates. Fix by limiting to a reasonable value. Spotted with Coverity. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/gigaset/capi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c index 044392cba262..47e2a913a6ef 100644 --- a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@ -250,6 +250,8 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, l -= 12; if (l <= 0) return; + if (l > 64) + l = 64; /* arbitrary limit */ dbgline = kmalloc(3 * l, GFP_ATOMIC); if (!dbgline) return; From 846ac30135e7c5e03b487c16c87ccb1ab020a01f Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:29 +0200 Subject: [PATCH 04/12] isdn/gigaset: fix NULL pointer dereference In do_action, a NULL pointer might be passed to function start_dial which will dereference it. Fix by adding a check for NULL before the call. Spotted with Coverity. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/gigaset/ev-layer.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c index dcae14aef376..0f699ebd81c5 100644 --- a/drivers/isdn/gigaset/ev-layer.c +++ b/drivers/isdn/gigaset/ev-layer.c @@ -1380,6 +1380,11 @@ static void do_action(int action, struct cardstate *cs, /* events from the LL */ case ACT_DIAL: + if (!ev->ptr) { + *p_genresp = 1; + *p_resp_code = RSP_ERROR; + break; + } start_dial(at_state, ev->ptr, ev->parameter); break; case ACT_ACCEPT: From b8324f94202af7dc688576259803a2ef9a98d655 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 05/12] isdn/gigaset: fix non-heap pointer deallocation at_state structures may be allocated individually or as part of a cardstate or bc_state structure. The disconnect() function handled both cases, creating a risk that it might try to deallocate an at_state structure that had not been allocated individually. Fix by splitting disconnect() into two variants handling cases with and without an associated B channel separately, and adding an explicit check. Spotted with Coverity. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/gigaset/ev-layer.c | 111 ++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 41 deletions(-) diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c index 0f699ebd81c5..c8ced12fa452 100644 --- a/drivers/isdn/gigaset/ev-layer.c +++ b/drivers/isdn/gigaset/ev-layer.c @@ -604,14 +604,14 @@ void gigaset_handle_modem_response(struct cardstate *cs) } EXPORT_SYMBOL_GPL(gigaset_handle_modem_response); -/* disconnect +/* disconnect_nobc * process closing of connection associated with given AT state structure + * without B channel */ -static void disconnect(struct at_state_t **at_state_p) +static void disconnect_nobc(struct at_state_t **at_state_p, + struct cardstate *cs) { unsigned long flags; - struct bc_state *bcs = (*at_state_p)->bcs; - struct cardstate *cs = (*at_state_p)->cs; spin_lock_irqsave(&cs->lock, flags); ++(*at_state_p)->seq_index; @@ -622,23 +622,44 @@ static void disconnect(struct at_state_t **at_state_p) gig_dbg(DEBUG_EVENT, "Scheduling PC_UMMODE"); cs->commands_pending = 1; } - spin_unlock_irqrestore(&cs->lock, flags); - if (bcs) { - /* B channel assigned: invoke hardware specific handler */ - cs->ops->close_bchannel(bcs); - /* notify LL */ - if (bcs->chstate & (CHS_D_UP | CHS_NOTIFY_LL)) { - bcs->chstate &= ~(CHS_D_UP | CHS_NOTIFY_LL); - gigaset_isdn_hupD(bcs); - } - } else { - /* no B channel assigned: just deallocate */ - spin_lock_irqsave(&cs->lock, flags); + /* check for and deallocate temporary AT state */ + if (!list_empty(&(*at_state_p)->list)) { list_del(&(*at_state_p)->list); kfree(*at_state_p); *at_state_p = NULL; - spin_unlock_irqrestore(&cs->lock, flags); + } + + spin_unlock_irqrestore(&cs->lock, flags); +} + +/* disconnect_bc + * process closing of connection associated with given AT state structure + * and B channel + */ +static void disconnect_bc(struct at_state_t *at_state, + struct cardstate *cs, struct bc_state *bcs) +{ + unsigned long flags; + + spin_lock_irqsave(&cs->lock, flags); + ++at_state->seq_index; + + /* revert to selected idle mode */ + if (!cs->cidmode) { + cs->at_state.pending_commands |= PC_UMMODE; + gig_dbg(DEBUG_EVENT, "Scheduling PC_UMMODE"); + cs->commands_pending = 1; + } + spin_unlock_irqrestore(&cs->lock, flags); + + /* invoke hardware specific handler */ + cs->ops->close_bchannel(bcs); + + /* notify LL */ + if (bcs->chstate & (CHS_D_UP | CHS_NOTIFY_LL)) { + bcs->chstate &= ~(CHS_D_UP | CHS_NOTIFY_LL); + gigaset_isdn_hupD(bcs); } } @@ -646,7 +667,7 @@ static void disconnect(struct at_state_t **at_state_p) * get a free AT state structure: either one of those associated with the * B channels of the Gigaset device, or if none of those is available, * a newly allocated one with bcs=NULL - * The structure should be freed by calling disconnect() after use. + * The structure should be freed by calling disconnect_nobc() after use. */ static inline struct at_state_t *get_free_channel(struct cardstate *cs, int cid) @@ -1057,7 +1078,7 @@ static void do_action(int action, struct cardstate *cs, struct event_t *ev) { struct at_state_t *at_state = *p_at_state; - struct at_state_t *at_state2; + struct bc_state *bcs2; unsigned long flags; int channel; @@ -1156,8 +1177,8 @@ static void do_action(int action, struct cardstate *cs, break; case ACT_RING: /* get fresh AT state structure for new CID */ - at_state2 = get_free_channel(cs, ev->parameter); - if (!at_state2) { + at_state = get_free_channel(cs, ev->parameter); + if (!at_state) { dev_warn(cs->dev, "RING ignored: could not allocate channel structure\n"); break; @@ -1166,16 +1187,16 @@ static void do_action(int action, struct cardstate *cs, /* initialize AT state structure * note that bcs may be NULL if no B channel is free */ - at_state2->ConState = 700; + at_state->ConState = 700; for (i = 0; i < STR_NUM; ++i) { - kfree(at_state2->str_var[i]); - at_state2->str_var[i] = NULL; + kfree(at_state->str_var[i]); + at_state->str_var[i] = NULL; } - at_state2->int_var[VAR_ZCTP] = -1; + at_state->int_var[VAR_ZCTP] = -1; spin_lock_irqsave(&cs->lock, flags); - at_state2->timer_expires = RING_TIMEOUT; - at_state2->timer_active = 1; + at_state->timer_expires = RING_TIMEOUT; + at_state->timer_active = 1; spin_unlock_irqrestore(&cs->lock, flags); break; case ACT_ICALL: @@ -1213,14 +1234,17 @@ static void do_action(int action, struct cardstate *cs, case ACT_DISCONNECT: cs->cur_at_seq = SEQ_NONE; at_state->cid = -1; - if (bcs && cs->onechannel && cs->dle) { + if (!bcs) { + disconnect_nobc(p_at_state, cs); + } else if (cs->onechannel && cs->dle) { /* Check for other open channels not needed: * DLE only used for M10x with one B channel. */ at_state->pending_commands |= PC_DLE0; cs->commands_pending = 1; - } else - disconnect(p_at_state); + } else { + disconnect_bc(at_state, cs, bcs); + } break; case ACT_FAKEDLE0: at_state->int_var[VAR_ZDLE] = 0; @@ -1228,25 +1252,27 @@ static void do_action(int action, struct cardstate *cs, /* fall through */ case ACT_DLE0: cs->cur_at_seq = SEQ_NONE; - at_state2 = &cs->bcs[cs->curchannel].at_state; - disconnect(&at_state2); + bcs2 = cs->bcs + cs->curchannel; + disconnect_bc(&bcs2->at_state, cs, bcs2); break; case ACT_ABORTHUP: cs->cur_at_seq = SEQ_NONE; dev_warn(cs->dev, "Could not hang up.\n"); at_state->cid = -1; - if (bcs && cs->onechannel) + if (!bcs) + disconnect_nobc(p_at_state, cs); + else if (cs->onechannel) at_state->pending_commands |= PC_DLE0; else - disconnect(p_at_state); + disconnect_bc(at_state, cs, bcs); schedule_init(cs, MS_RECOVER); break; case ACT_FAILDLE0: cs->cur_at_seq = SEQ_NONE; dev_warn(cs->dev, "Error leaving DLE mode.\n"); cs->dle = 0; - at_state2 = &cs->bcs[cs->curchannel].at_state; - disconnect(&at_state2); + bcs2 = cs->bcs + cs->curchannel; + disconnect_bc(&bcs2->at_state, cs, bcs2); schedule_init(cs, MS_RECOVER); break; case ACT_FAILDLE1: @@ -1275,14 +1301,14 @@ static void do_action(int action, struct cardstate *cs, if (reinit_and_retry(cs, channel) < 0) { dev_warn(cs->dev, "Could not get a call ID. Cannot dial.\n"); - at_state2 = &cs->bcs[channel].at_state; - disconnect(&at_state2); + bcs2 = cs->bcs + channel; + disconnect_bc(&bcs2->at_state, cs, bcs2); } break; case ACT_ABORTCID: cs->cur_at_seq = SEQ_NONE; - at_state2 = &cs->bcs[cs->curchannel].at_state; - disconnect(&at_state2); + bcs2 = cs->bcs + cs->curchannel; + disconnect_bc(&bcs2->at_state, cs, bcs2); break; case ACT_DIALING: @@ -1291,7 +1317,10 @@ static void do_action(int action, struct cardstate *cs, break; case ACT_ABORTACCEPT: /* hangup/error/timeout during ICALL procssng */ - disconnect(p_at_state); + if (bcs) + disconnect_bc(at_state, cs, bcs); + else + disconnect_nobc(p_at_state, cs); break; case ACT_ABORTDIAL: /* error/timeout during dial preparation */ From 9ea8aa8d5087529210553114b7bc4bf4374ace8f Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 06/12] isdn/capi: correct capi20_manufacturer argument type mismatch Function capi20_manufacturer() is declared with unsigned int cmd argument but called with unsigned long. Fix by correcting the function prototype since the actual argument is part of the user visible API. Spotted with Coverity. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/capi/kcapi.c | 4 ++-- include/linux/kernelcapi.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c index c123709acf82..823f6985b260 100644 --- a/drivers/isdn/capi/kcapi.c +++ b/drivers/isdn/capi/kcapi.c @@ -1184,7 +1184,7 @@ static int old_capi_manufacturer(unsigned int cmd, void __user *data) * Return value: CAPI result code */ -int capi20_manufacturer(unsigned int cmd, void __user *data) +int capi20_manufacturer(unsigned long cmd, void __user *data) { struct capi_ctr *ctr; int retval; @@ -1259,7 +1259,7 @@ int capi20_manufacturer(unsigned int cmd, void __user *data) } default: - printk(KERN_ERR "kcapi: manufacturer command %d unknown.\n", + printk(KERN_ERR "kcapi: manufacturer command %lu unknown.\n", cmd); break; diff --git a/include/linux/kernelcapi.h b/include/linux/kernelcapi.h index 9be37da93680..e985ba679c4a 100644 --- a/include/linux/kernelcapi.h +++ b/include/linux/kernelcapi.h @@ -41,7 +41,7 @@ u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN]); u16 capi20_get_version(u32 contr, struct capi_version *verp); u16 capi20_get_serial(u32 contr, u8 serial[CAPI_SERIAL_LEN]); u16 capi20_get_profile(u32 contr, struct capi_profile *profp); -int capi20_manufacturer(unsigned int cmd, void __user *data); +int capi20_manufacturer(unsigned long cmd, void __user *data); #define CAPICTR_UP 0 #define CAPICTR_DOWN 1 From 5362247a42e18ef74e698bb23575c272f8e35375 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 07/12] isdn/capi: prevent index overrun from command_2_index() The result of the function command_2_index() is used to index two arrays mnames[] and cpars[] with max. index 0x4e but in its current form that function can produce results up to 3*(0x9+0x9)+0x7f = 0xb5. Fix by clamping all result values potentially overrunning the arrays to zero which is already handled as an invalid value. Re-spotted with Coverity. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/capi/capiutil.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/isdn/capi/capiutil.c b/drivers/isdn/capi/capiutil.c index 4073d1684d07..b501d76209f0 100644 --- a/drivers/isdn/capi/capiutil.c +++ b/drivers/isdn/capi/capiutil.c @@ -207,6 +207,8 @@ static unsigned command_2_index(unsigned c, unsigned sc) c = 0x9 + (c & 0x0f); else if (c == 0x41) c = 0x9 + 0x1; + if (c > 0x18) + c = 0x00; return (sc & 3) * (0x9 + 0x9) + c; } From 854d23b77aa25b203c7af11de885c3b8b3834c20 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 08/12] isdn/capi: refactor command/subcommand table accesses Encapsulate accesses to the CAPI 2.0 command/subcommand name and parameter tables in a single place in preparation for redesign. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/capi/capiutil.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/drivers/isdn/capi/capiutil.c b/drivers/isdn/capi/capiutil.c index b501d76209f0..8e401eda2aa1 100644 --- a/drivers/isdn/capi/capiutil.c +++ b/drivers/isdn/capi/capiutil.c @@ -212,6 +212,19 @@ static unsigned command_2_index(unsigned c, unsigned sc) return (sc & 3) * (0x9 + 0x9) + c; } +/** + * capi_cmd2par() - find parameter string for CAPI 2.0 command/subcommand + * @cmd: command number + * @subcmd: subcommand number + * + * Return value: static string, NULL if command/subcommand unknown + */ + +static unsigned char *capi_cmd2par(u8 cmd, u8 subcmd) +{ + return cpars[command_2_index(cmd, subcmd)]; +} + /*-------------------------------------------------------*/ #define TYP (cdef[cmsg->par[cmsg->p]].typ) #define OFF (((u8 *)cmsg) + cdef[cmsg->par[cmsg->p]].off) @@ -304,7 +317,7 @@ unsigned capi_cmsg2message(_cmsg *cmsg, u8 *msg) cmsg->m = msg; cmsg->l = 8; cmsg->p = 0; - cmsg->par = cpars[command_2_index(cmsg->Command, cmsg->Subcommand)]; + cmsg->par = capi_cmd2par(cmsg->Command, cmsg->Subcommand); pars_2_message(cmsg); @@ -377,7 +390,7 @@ unsigned capi_message2cmsg(_cmsg *cmsg, u8 *msg) cmsg->p = 0; byteTRcpy(cmsg->m + 4, &cmsg->Command); byteTRcpy(cmsg->m + 5, &cmsg->Subcommand); - cmsg->par = cpars[command_2_index(cmsg->Command, cmsg->Subcommand)]; + cmsg->par = capi_cmd2par(cmsg->Command, cmsg->Subcommand); message_2_pars(cmsg); @@ -761,10 +774,10 @@ _cdebbuf *capi_message2str(u8 *msg) cmsg->p = 0; byteTRcpy(cmsg->m + 4, &cmsg->Command); byteTRcpy(cmsg->m + 5, &cmsg->Subcommand); - cmsg->par = cpars[command_2_index(cmsg->Command, cmsg->Subcommand)]; + cmsg->par = capi_cmd2par(cmsg->Command, cmsg->Subcommand); cdb = bufprint(cdb, "%-26s ID=%03d #0x%04x LEN=%04d\n", - mnames[command_2_index(cmsg->Command, cmsg->Subcommand)], + capi_cmd2str(cmsg->Command, cmsg->Subcommand), ((unsigned short *) msg)[1], ((unsigned short *) msg)[3], ((unsigned short *) msg)[0]); @@ -798,7 +811,7 @@ _cdebbuf *capi_cmsg2str(_cmsg *cmsg) cmsg->l = 8; cmsg->p = 0; cdb = bufprint(cdb, "%s ID=%03d #0x%04x LEN=%04d\n", - mnames[command_2_index(cmsg->Command, cmsg->Subcommand)], + capi_cmd2str(cmsg->Command, cmsg->Subcommand), ((u16 *) cmsg->m)[1], ((u16 *) cmsg->m)[3], ((u16 *) cmsg->m)[0]); From 5510ab18048397193ae073d6b0d4ea78ff0170f5 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 09/12] isdn/capi: prevent NULL pointer dereference on invalid CAPI command An invalid CAPI 2.0 command/subcommand combination may retrieve a NULL pointer from the cpars[] array which will later be dereferenced by the parser routines. Fix by adding NULL pointer checks in strategic places. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/capi/capiutil.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/isdn/capi/capiutil.c b/drivers/isdn/capi/capiutil.c index 8e401eda2aa1..36835ef3f340 100644 --- a/drivers/isdn/capi/capiutil.c +++ b/drivers/isdn/capi/capiutil.c @@ -318,6 +318,8 @@ unsigned capi_cmsg2message(_cmsg *cmsg, u8 *msg) cmsg->l = 8; cmsg->p = 0; cmsg->par = capi_cmd2par(cmsg->Command, cmsg->Subcommand); + if (!cmsg->par) + return 1; /* invalid command/subcommand */ pars_2_message(cmsg); @@ -391,6 +393,8 @@ unsigned capi_message2cmsg(_cmsg *cmsg, u8 *msg) byteTRcpy(cmsg->m + 4, &cmsg->Command); byteTRcpy(cmsg->m + 5, &cmsg->Subcommand); cmsg->par = capi_cmd2par(cmsg->Command, cmsg->Subcommand); + if (!cmsg->par) + return 1; /* invalid command/subcommand */ message_2_pars(cmsg); @@ -640,6 +644,9 @@ static _cdebbuf *printstruct(_cdebbuf *cdb, u8 *m) static _cdebbuf *protocol_message_2_pars(_cdebbuf *cdb, _cmsg *cmsg, int level) { + if (!cmsg->par) + return NULL; /* invalid command/subcommand */ + for (; TYP != _CEND; cmsg->p++) { int slen = 29 + 3 - level; int i; From 2bf3a09ea51f807d78d48d0ebc591b9e1502a743 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 10/12] isdn/capi: handle CAPI 2.0 message parser failures Have callers of capi_cmsg2message and capi_message2cmsg handle non-zero return values indicating failure. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/capi/capidrv.c | 24 +++++- drivers/isdn/gigaset/capi.c | 148 ++++++++++++++++++++++++++++++------ 2 files changed, 145 insertions(+), 27 deletions(-) diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c index fd6d28f3fc36..1cc6ca8bfbda 100644 --- a/drivers/isdn/capi/capidrv.c +++ b/drivers/isdn/capi/capidrv.c @@ -506,7 +506,10 @@ static void send_message(capidrv_contr *card, _cmsg *cmsg) struct sk_buff *skb; size_t len; - capi_cmsg2message(cmsg, cmsg->buf); + if (capi_cmsg2message(cmsg, cmsg->buf)) { + printk(KERN_ERR "capidrv::send_message: parser failure\n"); + return; + } len = CAPIMSG_LEN(cmsg->buf); skb = alloc_skb(len, GFP_ATOMIC); if (!skb) { @@ -1578,7 +1581,12 @@ static _cmsg s_cmsg; static void capidrv_recv_message(struct capi20_appl *ap, struct sk_buff *skb) { - capi_message2cmsg(&s_cmsg, skb->data); + if (capi_message2cmsg(&s_cmsg, skb->data)) { + printk(KERN_ERR "capidrv: applid=%d: received invalid message\n", + ap->applid); + kfree_skb(skb); + return; + } if (debugmode > 3) { _cdebbuf *cdb = capi_cmsg2str(&s_cmsg); @@ -1903,7 +1911,11 @@ static int capidrv_command(isdn_ctrl *c, capidrv_contr *card) NULL, /* Useruserdata */ NULL /* Facilitydataarray */ ); - capi_cmsg2message(&cmdcmsg, cmdcmsg.buf); + if (capi_cmsg2message(&cmdcmsg, cmdcmsg.buf)) { + printk(KERN_ERR "capidrv-%d: capidrv_command: parser failure\n", + card->contrnr); + return -EINVAL; + } plci_change_state(card, bchan->plcip, EV_PLCI_CONNECT_RESP); send_message(card, &cmdcmsg); return 0; @@ -2090,7 +2102,11 @@ static int if_sendbuf(int id, int channel, int doack, struct sk_buff *skb) if (capidrv_add_ack(nccip, datahandle, doack ? (int)skb->len : -1) < 0) return 0; - capi_cmsg2message(&sendcmsg, sendcmsg.buf); + if (capi_cmsg2message(&sendcmsg, sendcmsg.buf)) { + printk(KERN_ERR "capidrv-%d: if_sendbuf: parser failure\n", + card->contrnr); + return -EINVAL; + } msglen = CAPIMSG_LEN(sendcmsg.buf); if (skb_headroom(skb) < msglen) { struct sk_buff *nskb = skb_realloc_headroom(skb, msglen); diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c index 47e2a913a6ef..ccec7778cad2 100644 --- a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@ -647,7 +647,13 @@ int gigaset_isdn_icall(struct at_state_t *at_state) __func__); break; } - capi_cmsg2message(&iif->hcmsg, __skb_put(skb, msgsize)); + if (capi_cmsg2message(&iif->hcmsg, + __skb_put(skb, msgsize))) { + dev_err(cs->dev, "%s: message parser failure\n", + __func__); + dev_kfree_skb_any(skb); + break; + } dump_cmsg(DEBUG_CMD, __func__, &iif->hcmsg); /* add to listeners on this B channel, update state */ @@ -693,7 +699,12 @@ static void send_disconnect_ind(struct bc_state *bcs, dev_err(cs->dev, "%s: out of memory\n", __func__); return; } - capi_cmsg2message(&iif->hcmsg, __skb_put(skb, CAPI_DISCONNECT_IND_LEN)); + if (capi_cmsg2message(&iif->hcmsg, + __skb_put(skb, CAPI_DISCONNECT_IND_LEN))) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->hcmsg); capi_ctr_handle_message(&iif->ctr, ap->id, skb); } @@ -723,8 +734,12 @@ static void send_disconnect_b3_ind(struct bc_state *bcs, dev_err(cs->dev, "%s: out of memory\n", __func__); return; } - capi_cmsg2message(&iif->hcmsg, - __skb_put(skb, CAPI_DISCONNECT_B3_IND_BASELEN)); + if (capi_cmsg2message(&iif->hcmsg, + __skb_put(skb, CAPI_DISCONNECT_B3_IND_BASELEN))) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->hcmsg); capi_ctr_handle_message(&iif->ctr, ap->id, skb); } @@ -789,7 +804,11 @@ void gigaset_isdn_connD(struct bc_state *bcs) dev_err(cs->dev, "%s: out of memory\n", __func__); return; } - capi_cmsg2message(&iif->hcmsg, __skb_put(skb, msgsize)); + if (capi_cmsg2message(&iif->hcmsg, __skb_put(skb, msgsize))) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->hcmsg); capi_ctr_handle_message(&iif->ctr, ap->id, skb); } @@ -889,7 +908,11 @@ void gigaset_isdn_connB(struct bc_state *bcs) dev_err(cs->dev, "%s: out of memory\n", __func__); return; } - capi_cmsg2message(&iif->hcmsg, __skb_put(skb, msgsize)); + if (capi_cmsg2message(&iif->hcmsg, __skb_put(skb, msgsize))) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->hcmsg); capi_ctr_handle_message(&iif->ctr, ap->id, skb); } @@ -1096,13 +1119,19 @@ static void send_conf(struct gigaset_capi_ctr *iif, struct sk_buff *skb, u16 info) { + struct cardstate *cs = iif->ctr.driverdata; + /* * _CONF replies always only have NCCI and Info parameters * so they'll fit into the _REQ message skb */ capi_cmsg_answer(&iif->acmsg); iif->acmsg.Info = info; - capi_cmsg2message(&iif->acmsg, skb->data); + if (capi_cmsg2message(&iif->acmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } __skb_trim(skb, CAPI_STDCONF_LEN); dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg); capi_ctr_handle_message(&iif->ctr, ap->id, skb); @@ -1124,7 +1153,11 @@ static void do_facility_req(struct gigaset_capi_ctr *iif, static u8 confparam[10]; /* max. 9 octets + length byte */ /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); /* @@ -1223,6 +1256,7 @@ static void do_facility_req(struct gigaset_capi_ctr *iif, } /* send FACILITY_CONF with given Info and confirmation parameter */ + dev_kfree_skb_any(skb); capi_cmsg_answer(cmsg); cmsg->Info = info; cmsg->FacilityConfirmationParameter = confparam; @@ -1232,7 +1266,11 @@ static void do_facility_req(struct gigaset_capi_ctr *iif, dev_err(cs->dev, "%s: out of memory\n", __func__); return; } - capi_cmsg2message(cmsg, __skb_put(cskb, msgsize)); + if (capi_cmsg2message(cmsg, __skb_put(cskb, msgsize))) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(cskb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); capi_ctr_handle_message(&iif->ctr, ap->id, cskb); } @@ -1246,8 +1284,14 @@ static void do_listen_req(struct gigaset_capi_ctr *iif, struct gigaset_capi_appl *ap, struct sk_buff *skb) { + struct cardstate *cs = iif->ctr.driverdata; + /* decode message */ - capi_message2cmsg(&iif->acmsg, skb->data); + if (capi_message2cmsg(&iif->acmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg); /* store listening parameters */ @@ -1264,8 +1308,14 @@ static void do_alert_req(struct gigaset_capi_ctr *iif, struct gigaset_capi_appl *ap, struct sk_buff *skb) { + struct cardstate *cs = iif->ctr.driverdata; + /* decode message */ - capi_message2cmsg(&iif->acmsg, skb->data); + if (capi_message2cmsg(&iif->acmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg); send_conf(iif, ap, skb, CapiAlertAlreadySent); } @@ -1290,7 +1340,11 @@ static void do_connect_req(struct gigaset_capi_ctr *iif, u16 info; /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); /* get free B channel & construct PLCI */ @@ -1577,7 +1631,11 @@ static void do_connect_resp(struct gigaset_capi_ctr *iif, int channel; /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); dev_kfree_skb_any(skb); @@ -1743,7 +1801,11 @@ static void do_connect_b3_req(struct gigaset_capi_ctr *iif, int channel; /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); /* extract and check channel number from PLCI */ @@ -1788,7 +1850,11 @@ static void do_connect_b3_resp(struct gigaset_capi_ctr *iif, u8 command; /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); /* extract and check channel number and NCCI */ @@ -1828,7 +1894,11 @@ static void do_connect_b3_resp(struct gigaset_capi_ctr *iif, capi_cmsg_header(cmsg, ap->id, command, CAPI_IND, ap->nextMessageNumber++, cmsg->adr.adrNCCI); __skb_trim(skb, msgsize); - capi_cmsg2message(cmsg, skb->data); + if (capi_cmsg2message(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); capi_ctr_handle_message(&iif->ctr, ap->id, skb); } @@ -1850,7 +1920,11 @@ static void do_disconnect_req(struct gigaset_capi_ctr *iif, int channel; /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); /* extract and check channel number from PLCI */ @@ -1906,8 +1980,14 @@ static void do_disconnect_req(struct gigaset_capi_ctr *iif, kfree(b3cmsg); return; } - capi_cmsg2message(b3cmsg, - __skb_put(b3skb, CAPI_DISCONNECT_B3_IND_BASELEN)); + if (capi_cmsg2message(b3cmsg, + __skb_put(b3skb, CAPI_DISCONNECT_B3_IND_BASELEN))) { + dev_err(cs->dev, "%s: message parser failure\n", + __func__); + kfree(b3cmsg); + dev_kfree_skb_any(b3skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, b3cmsg); kfree(b3cmsg); capi_ctr_handle_message(&iif->ctr, ap->id, b3skb); @@ -1938,7 +2018,11 @@ static void do_disconnect_b3_req(struct gigaset_capi_ctr *iif, int channel; /* decode message */ - capi_message2cmsg(cmsg, skb->data); + if (capi_message2cmsg(cmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, cmsg); /* extract and check channel number and NCCI */ @@ -2055,8 +2139,14 @@ static void do_reset_b3_req(struct gigaset_capi_ctr *iif, struct gigaset_capi_appl *ap, struct sk_buff *skb) { + struct cardstate *cs = iif->ctr.driverdata; + /* decode message */ - capi_message2cmsg(&iif->acmsg, skb->data); + if (capi_message2cmsg(&iif->acmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg); send_conf(iif, ap, skb, CapiResetProcedureNotSupportedByCurrentProtocol); @@ -2069,8 +2159,14 @@ static void do_unsupported(struct gigaset_capi_ctr *iif, struct gigaset_capi_appl *ap, struct sk_buff *skb) { + struct cardstate *cs = iif->ctr.driverdata; + /* decode message */ - capi_message2cmsg(&iif->acmsg, skb->data); + if (capi_message2cmsg(&iif->acmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg); send_conf(iif, ap, skb, CapiMessageNotSupportedInCurrentState); } @@ -2082,8 +2178,14 @@ static void do_nothing(struct gigaset_capi_ctr *iif, struct gigaset_capi_appl *ap, struct sk_buff *skb) { + struct cardstate *cs = iif->ctr.driverdata; + /* decode message */ - capi_message2cmsg(&iif->acmsg, skb->data); + if (capi_message2cmsg(&iif->acmsg, skb->data)) { + dev_err(cs->dev, "%s: message parser failure\n", __func__); + dev_kfree_skb_any(skb); + return; + } dump_cmsg(DEBUG_CMD, __func__, &iif->acmsg); dev_kfree_skb_any(skb); } From 340184b35ac8786bdb574d2c8ce8e4f1269ec4da Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 11/12] isdn/capi: don't return NULL from capi_cmd2str() capi_cmd2str() is used in many places to build log messages. None of them is prepared to handle NULL as a result. Change the function to return printable string "INVALID_COMMAND" instead. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/capi/capiutil.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/isdn/capi/capiutil.c b/drivers/isdn/capi/capiutil.c index 36835ef3f340..36c1b37cea0a 100644 --- a/drivers/isdn/capi/capiutil.c +++ b/drivers/isdn/capi/capiutil.c @@ -489,12 +489,17 @@ static char *mnames[] = * @cmd: command number * @subcmd: subcommand number * - * Return value: static string, NULL if command/subcommand unknown + * Return value: static string */ char *capi_cmd2str(u8 cmd, u8 subcmd) { - return mnames[command_2_index(cmd, subcmd)]; + char *result; + + result = mnames[command_2_index(cmd, subcmd)]; + if (result == NULL) + result = "INVALID_COMMAND"; + return result; } From 86f8ef2c4802ac9dbe0c8c1c12670bd915a13013 Mon Sep 17 00:00:00 2001 From: Tilman Schmidt Date: Sat, 11 Oct 2014 13:46:30 +0200 Subject: [PATCH 12/12] isdn/gigaset: fix usb_gigaset write_cmd result race In usb_gigaset function gigaset_write_cmd(), the length field of the command buffer structure could be cleared by the transmit tasklet before it was used for the function's return value. Fix by copying to a local variable before scheduling the tasklet. Signed-off-by: Tilman Schmidt Signed-off-by: David S. Miller --- drivers/isdn/gigaset/usb-gigaset.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/isdn/gigaset/usb-gigaset.c b/drivers/isdn/gigaset/usb-gigaset.c index 82e91ba1acd3..a8e652dac54d 100644 --- a/drivers/isdn/gigaset/usb-gigaset.c +++ b/drivers/isdn/gigaset/usb-gigaset.c @@ -497,6 +497,7 @@ static int send_cb(struct cardstate *cs, struct cmdbuf_t *cb) static int gigaset_write_cmd(struct cardstate *cs, struct cmdbuf_t *cb) { unsigned long flags; + int len; gigaset_dbg_buffer(cs->mstate != MS_LOCKED ? DEBUG_TRANSCMD : DEBUG_LOCKCMD, @@ -515,10 +516,11 @@ static int gigaset_write_cmd(struct cardstate *cs, struct cmdbuf_t *cb) spin_unlock_irqrestore(&cs->cmdlock, flags); spin_lock_irqsave(&cs->lock, flags); + len = cb->len; if (cs->connected) tasklet_schedule(&cs->write_tasklet); spin_unlock_irqrestore(&cs->lock, flags); - return cb->len; + return len; } static int gigaset_write_room(struct cardstate *cs)