Coverity patches for drivers/isdn

PATCH In review Tilman Schmidt 2 participants 14 messages Oct 11, 2014 – Oct 14, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Tilman Schmidt

author

2014-10-11 11:47 UTC #0 lore raw

cover Coverity patches for drivers/isdn

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(-)

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #1 lore raw

PATCH 1/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 <[email protected]>

Signed-off-by Tilman Schmidt <[email protected]>

---
 drivers/isdn/gigaset/capi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/isdn/gigaset/capi.c +1 -0

index 3286903..a2eabe9 100644
@@ -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 */

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #2 lore raw

PATCH 2/12 isdn/gigaset: make sure controller name is null terminated

In reply to 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 <[email protected]>

---
 drivers/isdn/gigaset/capi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/isdn/gigaset/capi.c +2 -2

index a2eabe9..044392c 100644
@@ -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;

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #3 lore raw

PATCH 3/12 isdn/gigaset: limit raw CAPI message dump length

In reply to 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 <[email protected]>

---
 drivers/isdn/gigaset/capi.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/isdn/gigaset/capi.c +2 -0

index 044392c..47e2a91 100644
@@ -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;

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #4 lore raw

PATCH 4/12 isdn/gigaset: fix NULL pointer dereference

In reply to 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 <[email protected]>

---
 drivers/isdn/gigaset/ev-layer.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/isdn/gigaset/ev-layer.c +5 -0

index dcae14a..0f699eb 100644
@@ -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:

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #5 lore raw

PATCH 5/12 isdn/gigaset: fix non-heap pointer deallocation

In reply to 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 <[email protected]>

---
 drivers/isdn/gigaset/ev-layer.c | 111 +++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 41 deletions(-)

drivers/isdn/gigaset/ev-layer.c +70 ~~-41~~

index 0f699eb..c8ced12 100644
@@ -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 */

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #6 lore raw

PATCH 6/12 isdn/capi: correct capi20_manufacturer argument type mismatch

In reply to 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 <[email protected]>

---
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(-)

drivers/isdn/capi/kcapi.c +2 -2

index c123709..823f698 100644
@@ -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;

include/linux/kernelcapi.h +1 -1

index 9be37da..e985ba6 100644
@@ -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

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #7 lore raw

PATCH 7/12 isdn/capi: prevent index overrun from command_2_index()

In reply to 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 <[email protected]>

---
 drivers/isdn/capi/capiutil.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/isdn/capi/capiutil.c +2 -0

index 4073d16..b501d76 100644
@@ -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;
 }

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #8 lore raw

PATCH 8/12 isdn/capi: refactor command/subcommand table accesses

In reply to 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 <[email protected]>

---
 drivers/isdn/capi/capiutil.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

drivers/isdn/capi/capiutil.c +18 -5

index b501d76..8e401ed 100644
@@ -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]);

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #9 lore raw

PATCH 9/12 isdn/capi: prevent NULL pointer dereference on invalid CAPI command

In reply to 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 <[email protected]>

---
 drivers/isdn/capi/capiutil.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/isdn/capi/capiutil.c +7 -0

index 8e401ed..36835ef 100644
@@ -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;

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/isdn/capi/capiutil.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/isdn/capi/capiutil.c +7 -2

index 36835ef..36c1b37 100644
@@ -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;
 }

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/isdn/gigaset/usb-gigaset.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/isdn/gigaset/usb-gigaset.c +3 -1

index 82e91ba..a8e652d 100644
@@ -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)

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2014-10-11 11:47 UTC #10 lore raw

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

In reply to Tilman Schmidt

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

Signed-off-by Tilman Schmidt <[email protected]>

---
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(-)

drivers/isdn/capi/capidrv.c +20 -4

index fd6d28f..1cc6ca8 100644
@@ -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);

drivers/isdn/gigaset/capi.c +125 ~~-23~~

291 lines not on this page · raw message

Signature · 1 line

1.9.2.459.g68773ac

David Miller

maintainer

2014-10-14 19:06 UTC #13 lore raw

In reply to Tilman Schmidt

From: Tilman Schmidt <[email protected]>
Date: Sat, 11 Oct 2014 13:46:29 +0200 (CEST)

Quoted 8 lines

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.