[PATCH 00/12] Coverity patches for drivers/isdn

[PATCH 00/12] Coverity patches for drivers/isdn

[Tilman Schmidt]

Here's a series of patches for the ISDN CAPI subsystem and the
Gigaset ISDN driver.
Patches 1 to 7 are specific fixes for Coverity warnings.
Patches 8 to 11 fix related problems with the handling of invalid
CAPI command codes I noticed while working on this.
Patch 12 fixes an unrelated problem I noticed during the subsequent
regression tests.
It would be great if these could still be merged.

Thanks,
Tilman

Tilman Schmidt (12):
  isdn/gigaset: missing break in do_facility_req
  isdn/gigaset: make sure controller name is null terminated
  isdn/gigaset: limit raw CAPI message dump length
  isdn/gigaset: fix NULL pointer dereference
  isdn/gigaset: fix non-heap pointer deallocation
  isdn/capi: correct capi20_manufacturer argument type mismatch
  isdn/capi: prevent index overrun from command_2_index()
  isdn/capi: refactor command/subcommand table accesses
  isdn/capi: prevent NULL pointer dereference on invalid CAPI command
  isdn/capi: handle CAPI 2.0 message parser failures
  isdn/capi: don't return NULL from capi_cmd2str()
  isdn/gigaset: fix usb_gigaset write_cmd result race

 drivers/isdn/capi/capidrv.c        |  24 +++++-
 drivers/isdn/capi/capiutil.c       |  41 ++++++++--
 drivers/isdn/capi/kcapi.c          |   4 +-
 drivers/isdn/gigaset/capi.c        | 155 +++++++++++++++++++++++++++++++------
 drivers/isdn/gigaset/ev-layer.c    | 116 +++++++++++++++++----------
 drivers/isdn/gigaset/usb-gigaset.c |   4 +-
 include/linux/kernelcapi.h         |   2 +-
 7 files changed, 265 insertions(+), 81 deletions(-)

-- 
1.9.2.459.g68773ac

[PATCH 02/12] isdn/gigaset: make sure controller name is null terminated

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 a2eabe9..044392c 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;
-- 
1.9.2.459.g68773ac

[PATCH 03/12] isdn/gigaset: limit raw CAPI message dump length

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 044392c..47e2a91 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;
-- 
1.9.2.459.g68773ac

[PATCH 01/12] isdn/gigaset: missing break in do_facility_req

[Tilman Schmidt]

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 <davej@redhat.com>
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 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 3286903..a2eabe9 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 */
-- 
1.9.2.459.g68773ac

[PATCH 04/12] isdn/gigaset: fix NULL pointer dereference

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 dcae14a..0f699eb 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:
-- 
1.9.2.459.g68773ac

[PATCH 12/12] isdn/gigaset: fix usb_gigaset write_cmd result race

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 82e91ba..a8e652d 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)
-- 
1.9.2.459.g68773ac

[PATCH 08/12] isdn/capi: refactor command/subcommand table accesses

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 b501d76..8e401ed 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]);
-- 
1.9.2.459.g68773ac

[PATCH 05/12] isdn/gigaset: fix non-heap pointer deallocation

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 0f699eb..c8ced12 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 */
-- 
1.9.2.459.g68773ac

[PATCH 09/12] isdn/capi: prevent NULL pointer dereference on invalid CAPI command

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 8e401ed..36835ef 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;
-- 
1.9.2.459.g68773ac

[PATCH 10/12] isdn/capi: handle CAPI 2.0 message parser failures

[Tilman Schmidt]

Have callers of capi_cmsg2message and capi_message2cmsg handle
non-zero return values indicating failure.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
Note: The issue about printk(KERN_ERR which checkpatch is complaining
about is present in many more places in the original source file. I
deliberately preferred keeping the style consistent within the file.

 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 fd6d28f..1cc6ca8 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 47e2a91..ccec777 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);
 }
-- 
1.9.2.459.g68773ac

[PATCH 07/12] isdn/capi: prevent index overrun from command_2_index()

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 4073d16..b501d76 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;
 }
 
-- 
1.9.2.459.g68773ac

[PATCH 11/12] isdn/capi: don't return NULL from capi_cmd2str()

[Tilman Schmidt]

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 <tilman@imap.cc>
---
 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 36835ef..36c1b37 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;
 }
 
 
-- 
1.9.2.459.g68773ac

[PATCH 06/12] isdn/capi: correct capi20_manufacturer argument type mismatch

[Tilman Schmidt]

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 <tilman@imap.cc>
---
Note: The issue about printk(KERN_ERR which checkpatch is complaining
about is already present in the original file in this and many other
places. Cleaning it up is outside the scope of this patch.

 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 c123709..823f698 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 9be37da..e985ba6 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
-- 
1.9.2.459.g68773ac

Re: [PATCH 00/12] Coverity patches for drivers/isdn

[David Miller]

From: Tilman Schmidt <tilman@imap.cc>
Date: Sat, 11 Oct 2014 13:46:29 +0200 (CEST)

> Here's a series of patches for the ISDN CAPI subsystem and the
> Gigaset ISDN driver.
> Patches 1 to 7 are specific fixes for Coverity warnings.
> Patches 8 to 11 fix related problems with the handling of invalid
> CAPI command codes I noticed while working on this.
> Patch 12 fixes an unrelated problem I noticed during the subsequent
> regression tests.
> It would be great if these could still be merged.

Series applied, thanks.