gigaset307x-common — Common discussions about the GigaSet driver

Re: [Gigaset307x-common] [PATCH] isdn/gigaset: fix NULL-deref at probe

[David M. <da...@da...>]

From: Johan Hovold <jo...@ke...>
Date: Mon, 13 Mar 2017 13:39:01 +0100

> Make sure to check the number of endpoints to avoid dereferencing a
> NULL-pointer should a malicious device lack endpoints.
> 
> Fixes: cf7776dc05b8 ("[PATCH] isdn4linux: Siemens Gigaset drivers -
> direct USB connection")
> Cc: stable <st...@vg...>	# 2.6.17
> Cc: Hansjoerg Lipp <hj...@we...>
> 
> Signed-off-by: Johan Hovold <jo...@ke...>

Applied.

[Gigaset307x-common] [PATCH] isdn/gigaset: fix NULL-deref at probe

[Johan H. <jo...@ke...>]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack endpoints.

Fixes: cf7776dc05b8 ("[PATCH] isdn4linux: Siemens Gigaset drivers -
direct USB connection")
Cc: stable <st...@vg...>	# 2.6.17
Cc: Hansjoerg Lipp <hj...@we...>

Signed-off-by: Johan Hovold <jo...@ke...>
---
 drivers/isdn/gigaset/bas-gigaset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/isdn/gigaset/bas-gigaset.c b/drivers/isdn/gigaset/bas-gigaset.c
index 11e13c56126f..2da3ff650e1d 100644
--- a/drivers/isdn/gigaset/bas-gigaset.c
+++ b/drivers/isdn/gigaset/bas-gigaset.c
@@ -2317,6 +2317,9 @@ static int gigaset_probe(struct usb_interface *interface,
 		return -ENODEV;
 	}
 
+	if (hostif->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	dev_info(&udev->dev,
 		 "%s: Device matched (Vendor: 0x%x, Product: 0x%x)\n",
 		 __func__, le16_to_cpu(udev->descriptor.idVendor),
-- 
2.12.0

Re: [Gigaset307x-common] [PATCH 20/35] drivers/isdn: Convert remaining uses of pr_warning to pr_warn

[Paul B. <pe...@ti...>]

On Thu, 2017-02-16 at 23:11 -0800, Joe Perches wrote:
> To enable eventual removal of pr_warning
> 
> This makes pr_warn use consistent for drivers/isdn
> 
> Prior to this patch, there were 20 uses of pr_warning and
> 3 uses of pr_warn in drivers/isdn
> 
> Signed-off-by: Joe Perches <jo...@pe...>
> ---
>  drivers/isdn/gigaset/interface.c        |  2 +-

Gigaset hunk looks good to me:
    Acked-by: Paul Bolle <pe...@ti...>

>  drivers/isdn/hardware/mISDN/avmfritz.c  | 17 +++++++++--------
>  drivers/isdn/hardware/mISDN/hfcmulti.c  |  8 ++++----
>  drivers/isdn/hardware/mISDN/hfcpci.c    |  4 ++--
>  drivers/isdn/hardware/mISDN/hfcsusb.c   |  4 ++--
>  drivers/isdn/hardware/mISDN/mISDNipac.c |  4 ++--
>  drivers/isdn/hardware/mISDN/mISDNisar.c | 10 +++++-----
>  drivers/isdn/hardware/mISDN/netjet.c    |  8 ++++----
>  drivers/isdn/hardware/mISDN/w6692.c     | 12 ++++++------
>  drivers/isdn/mISDN/hwchannel.c          |  8 ++++----
>  10 files changed, 39 insertions(+), 38 deletions(-)


Paul Bolle

[Gigaset307x-common] [PATCH 20/35] drivers/isdn: Convert remaining uses of pr_warning to pr_warn

[Joe P. <jo...@pe...>]

To enable eventual removal of pr_warning

This makes pr_warn use consistent for drivers/isdn

Prior to this patch, there were 20 uses of pr_warning and
3 uses of pr_warn in drivers/isdn

Signed-off-by: Joe Perches <jo...@pe...>
---
 drivers/isdn/gigaset/interface.c        |  2 +-
 drivers/isdn/hardware/mISDN/avmfritz.c  | 17 +++++++++--------
 drivers/isdn/hardware/mISDN/hfcmulti.c  |  8 ++++----
 drivers/isdn/hardware/mISDN/hfcpci.c    |  4 ++--
 drivers/isdn/hardware/mISDN/hfcsusb.c   |  4 ++--
 drivers/isdn/hardware/mISDN/mISDNipac.c |  4 ++--
 drivers/isdn/hardware/mISDN/mISDNisar.c | 10 +++++-----
 drivers/isdn/hardware/mISDN/netjet.c    |  8 ++++----
 drivers/isdn/hardware/mISDN/w6692.c     | 12 ++++++------
 drivers/isdn/mISDN/hwchannel.c          |  8 ++++----
 10 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/drivers/isdn/gigaset/interface.c b/drivers/isdn/gigaset/interface.c
index 600c79b030cd..4a2496d3f4ae 100644
--- a/drivers/isdn/gigaset/interface.c
+++ b/drivers/isdn/gigaset/interface.c
@@ -510,7 +510,7 @@ void gigaset_if_init(struct cardstate *cs)
 	if (!IS_ERR(cs->tty_dev))
 		dev_set_drvdata(cs->tty_dev, cs);
 	else {
-		pr_warning("could not register device to the tty subsystem\n");
+		pr_warn("could not register device to the tty subsystem\n");
 		cs->tty_dev = NULL;
 	}
 	mutex_unlock(&cs->mutex);
diff --git a/drivers/isdn/hardware/mISDN/avmfritz.c b/drivers/isdn/hardware/mISDN/avmfritz.c
index e3fa1cd64470..af88a0a7112c 100644
--- a/drivers/isdn/hardware/mISDN/avmfritz.c
+++ b/drivers/isdn/hardware/mISDN/avmfritz.c
@@ -414,8 +414,8 @@ hdlc_empty_fifo(struct bchannel *bch, int count)
 	} else {
 		cnt = bchannel_get_rxbuf(bch, count);
 		if (cnt < 0) {
-			pr_warning("%s.B%d: No bufferspace for %d bytes\n",
-				   fc->name, bch->nr, count);
+			pr_warn("%s.B%d: No bufferspace for %d bytes\n",
+				fc->name, bch->nr, count);
 			return;
 		}
 		p = skb_put(bch->rx_skb, count);
@@ -551,8 +551,8 @@ HDLC_irq(struct bchannel *bch, u32 stat)
 	}
 	if (stat & HDLC_INT_RPR) {
 		if (stat & HDLC_STAT_RDO) {
-			pr_warning("%s: ch%d stat %x RDO\n",
-				   fc->name, bch->nr, stat);
+			pr_warn("%s: ch%d stat %x RDO\n",
+				fc->name, bch->nr, stat);
 			hdlc->ctrl.sr.xml = 0;
 			hdlc->ctrl.sr.cmd |= HDLC_CMD_RRS;
 			write_ctrl(bch, 1);
@@ -574,8 +574,8 @@ HDLC_irq(struct bchannel *bch, u32 stat)
 				    HDLC_STAT_CRCVFR) {
 					recv_Bchannel(bch, 0, false);
 				} else {
-					pr_warning("%s: got invalid frame\n",
-						   fc->name);
+					pr_warn("%s: got invalid frame\n",
+						fc->name);
 					skb_trim(bch->rx_skb, 0);
 				}
 			}
@@ -587,8 +587,9 @@ HDLC_irq(struct bchannel *bch, u32 stat)
 		 * restart transmitting the whole frame on HDLC
 		 * in transparent mode we send the next data
 		 */
-		pr_warning("%s: ch%d stat %x XDU %s\n", fc->name, bch->nr,
-			   stat, bch->tx_skb ? "tx_skb" : "no tx_skb");
+		pr_warn("%s: ch%d stat %x XDU %s\n",
+			fc->name, bch->nr,
+			stat, bch->tx_skb ? "tx_skb" : "no tx_skb");
 		if (bch->tx_skb && bch->tx_skb->len) {
 			if (!test_bit(FLG_TRANSPARENT, &bch->Flags))
 				bch->tx_idx = 0;
diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 480c2d7794eb..7749b684acbc 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -2262,8 +2262,8 @@ hfcmulti_rx(struct hfc_multi *hc, int ch)
 	if (bch) {
 		maxlen = bchannel_get_rxbuf(bch, Zsize);
 		if (maxlen < 0) {
-			pr_warning("card%d.B%d: No bufferspace for %d bytes\n",
-				   hc->id + 1, bch->nr, Zsize);
+			pr_warn("card%d.B%d: No bufferspace for %d bytes\n",
+				hc->id + 1, bch->nr, Zsize);
 			return;
 		}
 		sp = &bch->rx_skb;
@@ -2274,8 +2274,8 @@ hfcmulti_rx(struct hfc_multi *hc, int ch)
 		if (*sp == NULL) {
 			*sp = mI_alloc_skb(maxlen, GFP_ATOMIC);
 			if (*sp == NULL) {
-				pr_warning("card%d: No mem for dch rx_skb\n",
-					   hc->id + 1);
+				pr_warn("card%d: No mem for dch rx_skb\n",
+					hc->id + 1);
 				return;
 			}
 		}
diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index ff48da61c94c..4d4fa6cb6412 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -579,8 +579,8 @@ hfcpci_empty_fifo_trans(struct bchannel *bch, struct bzfifo *rxbz,
 	}
 	maxlen = bchannel_get_rxbuf(bch, fcnt_rx);
 	if (maxlen < 0) {
-		pr_warning("B%d: No bufferspace for %d bytes\n",
-			   bch->nr, fcnt_rx);
+		pr_warn("B%d: No bufferspace for %d bytes\n",
+			bch->nr, fcnt_rx);
 	} else {
 		ptr = skb_put(bch->rx_skb, fcnt_rx);
 		if (le16_to_cpu(*z2r) + fcnt_rx <= B_FIFO_SIZE + B_SUB_VAL)
diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c
index 114f3bcba1b0..f9eaff9b4803 100644
--- a/drivers/isdn/hardware/mISDN/hfcsusb.c
+++ b/drivers/isdn/hardware/mISDN/hfcsusb.c
@@ -852,8 +852,8 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data, unsigned int len,
 		if (maxlen < 0) {
 			if (rx_skb)
 				skb_trim(rx_skb, 0);
-			pr_warning("%s.B%d: No bufferspace for %d bytes\n",
-				   hw->name, fifo->bch->nr, len);
+			pr_warn("%s.B%d: No bufferspace for %d bytes\n",
+				hw->name, fifo->bch->nr, len);
 			spin_unlock(&hw->lock);
 			return;
 		}
diff --git a/drivers/isdn/hardware/mISDN/mISDNipac.c b/drivers/isdn/hardware/mISDN/mISDNipac.c
index 77dec28ba874..48b4225e9ba1 100644
--- a/drivers/isdn/hardware/mISDN/mISDNipac.c
+++ b/drivers/isdn/hardware/mISDN/mISDNipac.c
@@ -954,8 +954,8 @@ hscx_empty_fifo(struct hscx_hw *hscx, u8 count)
 		hscx_cmdr(hscx, 0x80); /* RMC */
 		if (hscx->bch.rx_skb)
 			skb_trim(hscx->bch.rx_skb, 0);
-		pr_warning("%s.B%d: No bufferspace for %d bytes\n",
-			   hscx->ip->name, hscx->bch.nr, count);
+		pr_warn("%s.B%d: No bufferspace for %d bytes\n",
+			hscx->ip->name, hscx->bch.nr, count);
 		return;
 	}
 	p = skb_put(hscx->bch.rx_skb, count);
diff --git a/drivers/isdn/hardware/mISDN/mISDNisar.c b/drivers/isdn/hardware/mISDN/mISDNisar.c
index feafa91c2ed9..d789378cd879 100644
--- a/drivers/isdn/hardware/mISDN/mISDNisar.c
+++ b/drivers/isdn/hardware/mISDN/mISDNisar.c
@@ -235,7 +235,7 @@ load_firmware(struct isar_hw *isar, const u8 *buf, int size)
 			goto reterror;
 		}
 		if (!poll_mbox(isar, 1000)) {
-			pr_warning("ISAR poll_mbox dkey failed\n");
+			pr_warn("ISAR poll_mbox dkey failed\n");
 			ret = -ETIME;
 			goto reterror;
 		}
@@ -445,8 +445,8 @@ isar_rcv_frame(struct isar_ch *ch)
 	case ISDN_P_B_MODEM_ASYNC:
 		maxlen = bchannel_get_rxbuf(&ch->bch, ch->is->clsb);
 		if (maxlen < 0) {
-			pr_warning("%s.B%d: No bufferspace for %d bytes\n",
-				   ch->is->name, ch->bch.nr, ch->is->clsb);
+			pr_warn("%s.B%d: No bufferspace for %d bytes\n",
+				ch->is->name, ch->bch.nr, ch->is->clsb);
 			ch->is->write_reg(ch->is->hw, ISAR_IIA, 0);
 			break;
 		}
@@ -456,8 +456,8 @@ isar_rcv_frame(struct isar_ch *ch)
 	case ISDN_P_B_HDLC:
 		maxlen = bchannel_get_rxbuf(&ch->bch, ch->is->clsb);
 		if (maxlen < 0) {
-			pr_warning("%s.B%d: No bufferspace for %d bytes\n",
-				   ch->is->name, ch->bch.nr, ch->is->clsb);
+			pr_warn("%s.B%d: No bufferspace for %d bytes\n",
+				ch->is->name, ch->bch.nr, ch->is->clsb);
 			ch->is->write_reg(ch->is->hw, ISAR_IIA, 0);
 			break;
 		}
diff --git a/drivers/isdn/hardware/mISDN/netjet.c b/drivers/isdn/hardware/mISDN/netjet.c
index afde4edef9ae..a21c5bcbf35c 100644
--- a/drivers/isdn/hardware/mISDN/netjet.c
+++ b/drivers/isdn/hardware/mISDN/netjet.c
@@ -393,8 +393,8 @@ read_dma(struct tiger_ch *bc, u32 idx, int cnt)
 	stat = bchannel_get_rxbuf(&bc->bch, cnt);
 	/* only transparent use the count here, HDLC overun is detected later */
 	if (stat == -ENOMEM) {
-		pr_warning("%s.B%d: No memory for %d bytes\n",
-			   card->name, bc->bch.nr, cnt);
+		pr_warn("%s.B%d: No memory for %d bytes\n",
+			card->name, bc->bch.nr, cnt);
 		return;
 	}
 	if (test_bit(FLG_TRANSPARENT, &bc->bch.Flags))
@@ -433,8 +433,8 @@ read_dma(struct tiger_ch *bc, u32 idx, int cnt)
 			recv_Bchannel(&bc->bch, 0, false);
 			stat = bchannel_get_rxbuf(&bc->bch, bc->bch.maxlen);
 			if (stat < 0) {
-				pr_warning("%s.B%d: No memory for %d bytes\n",
-					   card->name, bc->bch.nr, cnt);
+				pr_warn("%s.B%d: No memory for %d bytes\n",
+					card->name, bc->bch.nr, cnt);
 				return;
 			}
 		} else if (stat == -HDLC_CRC_ERROR) {
diff --git a/drivers/isdn/hardware/mISDN/w6692.c b/drivers/isdn/hardware/mISDN/w6692.c
index 3b067ea656bd..9be37546b210 100644
--- a/drivers/isdn/hardware/mISDN/w6692.c
+++ b/drivers/isdn/hardware/mISDN/w6692.c
@@ -485,8 +485,8 @@ W6692_empty_Bfifo(struct w6692_ch *wch, int count)
 		WriteW6692B(wch, W_B_CMDR, W_B_CMDR_RACK | W_B_CMDR_RACT);
 		if (wch->bch.rx_skb)
 			skb_trim(wch->bch.rx_skb, 0);
-		pr_warning("%s.B%d: No bufferspace for %d bytes\n",
-			   card->name, wch->bch.nr, count);
+		pr_warn("%s.B%d: No bufferspace for %d bytes\n",
+			card->name, wch->bch.nr, count);
 		return;
 	}
 	ptr = skb_put(wch->bch.rx_skb, count);
@@ -749,8 +749,8 @@ W6692B_interrupt(struct w6692_hw *card, int ch)
 				 wch->bch.nr, star);
 		}
 		if (star & W_B_STAR_XDOW) {
-			pr_warning("%s: B%d XDOW proto=%x\n", card->name,
-				   wch->bch.nr, wch->bch.state);
+			pr_warn("%s: B%d XDOW proto=%x\n",
+				card->name, wch->bch.nr, wch->bch.state);
 #ifdef ERROR_STATISTIC
 			wch->bch.err_xdu++;
 #endif
@@ -767,8 +767,8 @@ W6692B_interrupt(struct w6692_hw *card, int ch)
 			return; /* handle XDOW only once */
 	}
 	if (stat & W_B_EXI_XDUN) {
-		pr_warning("%s: B%d XDUN proto=%x\n", card->name,
-			   wch->bch.nr, wch->bch.state);
+		pr_warn("%s: B%d XDUN proto=%x\n",
+			card->name, wch->bch.nr, wch->bch.state);
 #ifdef ERROR_STATISTIC
 		wch->bch.err_xdu++;
 #endif
diff --git a/drivers/isdn/mISDN/hwchannel.c b/drivers/isdn/mISDN/hwchannel.c
index 84b4b0f7eb99..a79d7d3d1429 100644
--- a/drivers/isdn/mISDN/hwchannel.c
+++ b/drivers/isdn/mISDN/hwchannel.c
@@ -483,8 +483,8 @@ bchannel_get_rxbuf(struct bchannel *bch, int reqlen)
 	if (bch->rx_skb) {
 		len = skb_tailroom(bch->rx_skb);
 		if (len < reqlen) {
-			pr_warning("B%d no space for %d (only %d) bytes\n",
-				   bch->nr, reqlen, len);
+			pr_warn("B%d no space for %d (only %d) bytes\n",
+				bch->nr, reqlen, len);
 			if (test_bit(FLG_TRANSPARENT, &bch->Flags)) {
 				/* send what we have now and try a new buffer */
 				recv_Bchannel(bch, 0, true);
@@ -517,8 +517,8 @@ bchannel_get_rxbuf(struct bchannel *bch, int reqlen)
 	}
 	bch->rx_skb = mI_alloc_skb(len, GFP_ATOMIC);
 	if (!bch->rx_skb) {
-		pr_warning("B%d receive no memory for %d bytes\n",
-			   bch->nr, len);
+		pr_warn("B%d receive no memory for %d bytes\n",
+			bch->nr, len);
 		len = -ENOMEM;
 	}
 	return len;
-- 
2.10.0.rc2.1.g053435c

Re: [Gigaset307x-common] [PATCH] isdn/gigaset: use designated initializers

[David M. <da...@da...>]

From: Kees Cook <kee...@ch...>
Date: Fri, 16 Dec 2016 16:58:06 -0800

> Prepare to mark sensitive kernel structures for randomization by making
> sure they're using designated initializers. These were identified during
> allyesconfig builds of x86, arm, and arm64, with most initializer fixes
> extracted from grsecurity.
> 
> Signed-off-by: Kees Cook <kee...@ch...>

Applied.

[Gigaset307x-common] [PATCH] isdn/gigaset: use designated initializers

[Kees C. <kee...@ch...>]

Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.

Signed-off-by: Kees Cook <kee...@ch...>
---
 drivers/isdn/gigaset/bas-gigaset.c | 32 ++++++++++++++++----------------
 drivers/isdn/gigaset/ser-gigaset.c | 32 ++++++++++++++++----------------
 drivers/isdn/gigaset/usb-gigaset.c | 32 ++++++++++++++++----------------
 3 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/drivers/isdn/gigaset/bas-gigaset.c b/drivers/isdn/gigaset/bas-gigaset.c
index aecec6d32463..11e13c56126f 100644
--- a/drivers/isdn/gigaset/bas-gigaset.c
+++ b/drivers/isdn/gigaset/bas-gigaset.c
@@ -2565,22 +2565,22 @@ static int gigaset_post_reset(struct usb_interface *intf)
 
 
 static const struct gigaset_ops gigops = {
-	gigaset_write_cmd,
-	gigaset_write_room,
-	gigaset_chars_in_buffer,
-	gigaset_brkchars,
-	gigaset_init_bchannel,
-	gigaset_close_bchannel,
-	gigaset_initbcshw,
-	gigaset_freebcshw,
-	gigaset_reinitbcshw,
-	gigaset_initcshw,
-	gigaset_freecshw,
-	gigaset_set_modem_ctrl,
-	gigaset_baud_rate,
-	gigaset_set_line_ctrl,
-	gigaset_isoc_send_skb,
-	gigaset_isoc_input,
+	.write_cmd = gigaset_write_cmd,
+	.write_room = gigaset_write_room,
+	.chars_in_buffer = gigaset_chars_in_buffer,
+	.brkchars = gigaset_brkchars,
+	.init_bchannel = gigaset_init_bchannel,
+	.close_bchannel = gigaset_close_bchannel,
+	.initbcshw = gigaset_initbcshw,
+	.freebcshw = gigaset_freebcshw,
+	.reinitbcshw = gigaset_reinitbcshw,
+	.initcshw = gigaset_initcshw,
+	.freecshw = gigaset_freecshw,
+	.set_modem_ctrl = gigaset_set_modem_ctrl,
+	.baud_rate = gigaset_baud_rate,
+	.set_line_ctrl = gigaset_set_line_ctrl,
+	.send_skb = gigaset_isoc_send_skb,
+	.handle_input = gigaset_isoc_input,
 };
 
 /* bas_gigaset_init
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index b90776ef56ec..ab0b63a4d045 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -445,22 +445,22 @@ static int gigaset_set_line_ctrl(struct cardstate *cs, unsigned cflag)
 }
 
 static const struct gigaset_ops ops = {
-	gigaset_write_cmd,
-	gigaset_write_room,
-	gigaset_chars_in_buffer,
-	gigaset_brkchars,
-	gigaset_init_bchannel,
-	gigaset_close_bchannel,
-	gigaset_initbcshw,
-	gigaset_freebcshw,
-	gigaset_reinitbcshw,
-	gigaset_initcshw,
-	gigaset_freecshw,
-	gigaset_set_modem_ctrl,
-	gigaset_baud_rate,
-	gigaset_set_line_ctrl,
-	gigaset_m10x_send_skb,	/* asyncdata.c */
-	gigaset_m10x_input,	/* asyncdata.c */
+	.write_cmd = gigaset_write_cmd,
+	.write_room = gigaset_write_room,
+	.chars_in_buffer = gigaset_chars_in_buffer,
+	.brkchars = gigaset_brkchars,
+	.init_bchannel = gigaset_init_bchannel,
+	.close_bchannel = gigaset_close_bchannel,
+	.initbcshw = gigaset_initbcshw,
+	.freebcshw = gigaset_freebcshw,
+	.reinitbcshw = gigaset_reinitbcshw,
+	.initcshw = gigaset_initcshw,
+	.freecshw = gigaset_freecshw,
+	.set_modem_ctrl = gigaset_set_modem_ctrl,
+	.baud_rate = gigaset_baud_rate,
+	.set_line_ctrl = gigaset_set_line_ctrl,
+	.send_skb = gigaset_m10x_send_skb,	/* asyncdata.c */
+	.handle_input = gigaset_m10x_input,	/* asyncdata.c */
 };
 
 
diff --git a/drivers/isdn/gigaset/usb-gigaset.c b/drivers/isdn/gigaset/usb-gigaset.c
index 5f306e2eece5..eade36dafa34 100644
--- a/drivers/isdn/gigaset/usb-gigaset.c
+++ b/drivers/isdn/gigaset/usb-gigaset.c
@@ -862,22 +862,22 @@ static int gigaset_pre_reset(struct usb_interface *intf)
 }
 
 static const struct gigaset_ops ops = {
-	gigaset_write_cmd,
-	gigaset_write_room,
-	gigaset_chars_in_buffer,
-	gigaset_brkchars,
-	gigaset_init_bchannel,
-	gigaset_close_bchannel,
-	gigaset_initbcshw,
-	gigaset_freebcshw,
-	gigaset_reinitbcshw,
-	gigaset_initcshw,
-	gigaset_freecshw,
-	gigaset_set_modem_ctrl,
-	gigaset_baud_rate,
-	gigaset_set_line_ctrl,
-	gigaset_m10x_send_skb,
-	gigaset_m10x_input,
+	.write_cmd = gigaset_write_cmd,
+	.write_room = gigaset_write_room,
+	.chars_in_buffer = gigaset_chars_in_buffer,
+	.brkchars = gigaset_brkchars,
+	.init_bchannel = gigaset_init_bchannel,
+	.close_bchannel = gigaset_close_bchannel,
+	.initbcshw = gigaset_initbcshw,
+	.freebcshw = gigaset_freebcshw,
+	.reinitbcshw = gigaset_reinitbcshw,
+	.initcshw = gigaset_initcshw,
+	.freecshw = gigaset_freecshw,
+	.set_modem_ctrl = gigaset_set_modem_ctrl,
+	.baud_rate = gigaset_baud_rate,
+	.set_line_ctrl = gigaset_set_line_ctrl,
+	.send_skb = gigaset_m10x_send_skb,
+	.handle_input = gigaset_m10x_input,
 };
 
 /*
-- 
2.7.4


-- 
Kees Cook
Nexus Security

Re: [Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[Paul B. <pe...@ti...>]

On Wed, 2016-12-07 at 23:04 +0100, Tilman Schmidt wrote:
> Not sure how much evil that does beyond the WARN, but I agree it's
> worth investigating.

For the record: NULL pointer dereference in tty_unregister_ldisc() on module
unload. rmmod got killed, module refcount set -1, etc. My test box locked up
twice some random period after all that. So quite a bit of a mess, all in all.


Paul Bolle

Re: [Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[David M. <da...@da...>]

From: Dan Carpenter <dan...@or...>
Date: Wed, 7 Dec 2016 14:22:03 +0300

> If we can't allocate the resources in gigaset_initdriver() then we
> should return -ENOMEM instead of zero.
> 
> Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")
> Signed-off-by: Dan Carpenter <dan...@or...>

Applied and queued up for -stable, thanks.

Re: [Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[Tilman S. <ti...@im...>]

Hi Paul,

Am 07.12.2016 um 22:08 schrieb Paul Bolle:
> On Wed, 2016-12-07 at 21:57 +0100, Tilman Schmidt wrote:
>> Not much of a mess, I reckon. Everything that has been allocated and
>> registered up to that point is properly deallocated and unregistered.
>> The code just fails to tell the kernel that module initialization has
>> failed, so the module remains loaded even though it can never be
>> called because it isn't hooked anywhere. That's a nuisance and a
>> waste of RAM, but not much more.
> 
> Yes.
> 
> But then the removal of the module, which is the only reasonable thing to do
> after all this has happened, seems to trigger a WARN in driver_unregister().
> And it's that WARN that I think requires the entire stable song and dance.

Ah, yes, of course, because driver_unregister() has already been run
in the failure path of module_init and is now called a second time.
Not sure how much evil that does beyond the WARN, but I agree it's
worth investigating.

Best regards,
Tilman

-- 
Tilman Schmidt                              E-Mail: ti...@im...
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Re: [Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[Tilman S. <ti...@im...>]

Am 07.12.2016 um 20:06 schrieb Paul Bolle:
> On Wed, 2016-12-07 at 14:22 +0300, Dan Carpenter wrote:
>> If we can't allocate the resources in gigaset_initdriver() then we
>> should return -ENOMEM instead of zero.
> 
> That's entirely correct.

Agree.

> I'll be back (probably tomorrow) after a short test to see whether this really
> needs to go into stable. It almost certainly should, but I'd like to first see
> the mess the current code leaves behind once gigaset_initdriver() fails before
> saying so.

Not much of a mess, I reckon. Everything that has been allocated and
registered up to that point is properly deallocated and unregistered.
The code just fails to tell the kernel that module initialization has
failed, so the module remains loaded even though it can never be
called because it isn't hooked anywhere. That's a nuisance and a
waste of RAM, but not much more.

HTH
Tilman

-- 
Tilman Schmidt                              E-Mail: ti...@im...
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Re: [Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[Paul B. <pe...@ti...>]

Hi Tilman,

On Wed, 2016-12-07 at 21:57 +0100, Tilman Schmidt wrote:
> Not much of a mess, I reckon. Everything that has been allocated and
> registered up to that point is properly deallocated and unregistered.
> The code just fails to tell the kernel that module initialization has
> failed, so the module remains loaded even though it can never be
> called because it isn't hooked anywhere. That's a nuisance and a
> waste of RAM, but not much more.

Yes.

But then the removal of the module, which is the only reasonable thing to do
after all this has happened, seems to trigger a WARN in driver_unregister().
And it's that WARN that I think requires the entire stable song and dance.

Otherwise it would be, as far as I can tell, a hard to hit problem in an
obscure driver without any side effects.


Paul Bolle

Re: [Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[Paul B. <pe...@ti...>]

On Wed, 2016-12-07 at 14:22 +0300, Dan Carpenter wrote:
> If we can't allocate the resources in gigaset_initdriver() then we
> should return -ENOMEM instead of zero.

That's entirely correct.

> Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")
> Signed-off-by: Dan Carpenter <dan...@or...>
> ---
> Ancient code.

For ancient hardware.

I'll be back (probably tomorrow) after a short test to see whether this really
needs to go into stable. It almost certainly should, but I'd like to first see
the mess the current code leaves behind once gigaset_initdriver() fails before
saying so.

Thanks for reporting this!


Paul Bolle

[Gigaset307x-common] [patch] ser_gigaset: return -ENOMEM on error instead of success

[Dan C. <dan...@or...>]

If we can't allocate the resources in gigaset_initdriver() then we
should return -ENOMEM instead of zero.

Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")
Signed-off-by: Dan Carpenter <dan...@or...>
---
Ancient code.

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index d1f8ab915b15..b90776ef56ec 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -755,8 +755,10 @@ static int __init ser_gigaset_init(void)
 	driver = gigaset_initdriver(GIGASET_MINOR, GIGASET_MINORS,
 				    GIGASET_MODULENAME, GIGASET_DEVNAME,
 				    &ops, THIS_MODULE);
-	if (!driver)
+	if (!driver) {
+		rc = -ENOMEM;
 		goto error;
+	}
 
 	rc = tty_register_ldisc(N_GIGASET_M101, &gigaset_ldisc);
 	if (rc != 0) {

Re: [Gigaset307x-common] ISDN-Gigaset: Fine-tuning for three function implementations

[Paul B. <pe...@ti...>]

On Wed, 2016-09-28 at 18:50 +0200, SF Markus Elfring wrote:
> Would you like to look once more into an improved patch series for this
> software module a bit later?

I'm afraid I'm not looking forward to receiving an update of this
series, sorry.

Thanks,


Paul Bolle

Re: [Gigaset307x-common] [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[Paul B. <pe...@ti...>]

On Wed, 2016-09-28 at 18:38 +0200, SF Markus Elfring wrote:
> > I'm not going to change code just because some checker suggests to
> > do so.
> 
> The script "checkpatch.pl" can point information out like the
> following.
> 
> WARNING: Prefer kmalloc_array over kmalloc with multiply

Am I being trolled?


Paul Bolle

Re: [Gigaset307x-common] ISDN-Gigaset: Fine-tuning for three function implementations

[SF M. E. <el...@us...>]

> Two of the five patches introduced bugs. The rest of the series isn't
> free of various nits either. Of course, I was in no mood to be lenient
> when I looked at those three patches.
> 
> I won't take any of these patches, sorry.

Would you like to look once more into an improved patch series for this
software module a bit later?

Regards,
Markus

Re: [Gigaset307x-common] [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[SF M. E. <el...@us...>]

>> * Multiplications for the size determination of memory allocations
>>   indicated that array data structures should be processed.
>>   Thus use the corresponding function "kmalloc_array".
> 
> Was the current code incorrect?

I suggest to use a safer interface for array allocations.


> What makes kmalloc_array() better?

1. How do you think about the safety checks that this function provides?

2. Will you be also affected by further software evolution here?
   2016-07-26
   mm: faster kmalloc_array(), kcalloc()
   https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=91c6a05f72a996bee5133e76374ab3ad7d3b9b72


> I'm not going to change code just because some checker suggests to do so.

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply


>>   This issue was detected by using the Coccinelle software.
> 
> So? And which coccinelle script was actually used?

How do you think about to look into related information sources?
https://github.com/coccinelle/coccinelle/issues/81

Would you like to experiment any further with an excerpt?


@replacement1@
expression count, target;
type T;
@@
 target =
-         kmalloc(sizeof(T) * (count)
+         kmalloc_array(count, sizeof(T)
                        , ...);

@replacement2@
expression count, pointer, target;
@@
 target =
-         kmalloc(sizeof(*pointer) * (count)
+         kmalloc_array(count, sizeof(*pointer)
                        , ...);


> I couldn't spot a coccinelle script doing that in the current tree.

This is true for such a software update opportunity.


>> * Replace the specification of a data structure by a pointer dereference
>>   to make the corresponding size determination a bit safer according to
>>   the Linux coding style convention.
> 
> I'm not happy with you mixing this with the above, less trivial, change.

I find that it is a useful combination. - A parameter is adjusted together
with a special function name.


>> -	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
>> +	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
> 
> For "minors" the same holds as for "channels", above.
> 
> And you snuck in a parentheses change. That should have probably been
> merged with 5/5.

Would you prefer to add them in another update step?

Regards,
Markus

Re: [Gigaset307x-common] [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function implementations

[Paul B. <pe...@ti...>]

On Mon, 2016-09-26 at 17:37 +0200, SF Markus Elfring wrote:
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   Use kmalloc_array() in two functions
>   Improve another size determination in gigaset_initcs()
>   Delete an error message for a failed memory allocation
>   Release memory in gigaset_initcs() after an allocation failure
>   Enclose two expressions for the sizeof operator by parentheses
> 
>  drivers/isdn/gigaset/common.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)

Two of the five patches introduced bugs. The rest of the series isn't
free of various nits either. Of course, I was in no mood to be lenient
when I looked at those three patches.

I won't take any of these patches, sorry.


Paul Bolle

Re: [Gigaset307x-common] [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory allocation

[Paul B. <pe...@ti...>]

On Tue, 2016-09-27 at 12:57 +0200, Tilman Schmidt wrote:
> On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote:
> > Omit an extra message for a memory allocation failure in this
> > function.
> > 
> > Link:
> > http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-R
> > efactor_Strings-WSang_0.pdf
> > 
> > Signed-off-by: Markus Elfring <el...@us...>
> 
> The patch is fine but the link in the commit message is irrelevant.
> Please remove it.
> (Yes, I read through the whole presentation to verify that. It was fun,
> even.)

Agree.

Except this patch assumes the superfluous braces would be removed in
4/5. But it turns out that other patch must be dropped. It would have
been better to remove the braces in this patch.


Paul Bolle

Re: [Gigaset307x-common] [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[Paul B. <pe...@ti...>]

On Mon, 2016-09-26 at 17:38 +0200, SF Markus Elfring wrote:
> * Multiplications for the size determination of memory allocations
>   indicated that array data structures should be processed.
>   Thus use the corresponding function "kmalloc_array".

Was the current code incorrect? What makes kmalloc_array() better? None
of this is obvious to me.

I'm not going to change code just because some checker suggests to do
so.

>   This issue was detected by using the Coccinelle software.

So? And which coccinelle script was actually used? I couldn't spot a
coccinelle script doing that in the current tree.

> * Replace the specification of a data structure by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.

I'm not happy with you mixing this with the above, less trivial,
change.

> Signed-off-by: Markus Elfring <el...@us...>

> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -709,8 +709,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  
>  	cs->mode = M_UNKNOWN;
>  	cs->mstate = MS_UNINITIALIZED;
> -

Unrelated whitespace change.

> -	cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
> +	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);

For the record: "channels" is basically hardcoded in the three gigaset
hardware drivers.

>  	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
>  	if (!cs->bcs || !cs->inbuf) {
>  		pr_err("out of memory\n");
> @@ -1089,8 +1088,7 @@ struct gigaset_driver
> *gigaset_initdriver(unsigned minor, unsigned minors,
>  	drv->ops = ops;
>  	drv->owner = owner;
>  	INIT_LIST_HEAD(&drv->list);
> -

Again unrelated whitespace change.

> -	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
> +	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);

For "minors" the same holds as for "channels", above.

And you snuck in a parentheses change. That should have probably been
merged with 5/5.

>  	if (!drv->cs)
>  		goto error;


Paul Bolle

Re: [Gigaset307x-common] [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF M. E. <el...@us...>]

>> @@ -772,8 +775,9 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>>  
>>  	gig_dbg(DEBUG_INIT, "cs initialized");
>>  	return cs;
>> -
>> -error:
>> +free_bcs:
>> +	kfree(cs->bcs);
>> +report_failure:
>>  	gig_dbg(DEBUG_INIT, "failed");
>>  	gigaset_freecs(cs);
> 
> gigaset_freecs() is not a function I look at for the fun of it. But
> still, in it we find:
> 
> 	case 0: /* error in basic setup */
> 		[...]
> 		kfree(cs->inbuf);
> 		kfree(cs->bcs);
> 
> As far as I can tell we will call those two kfree()'s if we jump to
> "error". So, contrary to your analysis, I don't think we leak cs->bcs.

You are right.

Thanks that you pointed this source code place out again.

I imagined that the exception handling implementation could be more direct
somehow for a while. But this function takes extra care for data synchronisation
by a mutex.

Now I recognise also that this proposed update step "4" was inappropriate.
I'm sorry for the confusion I introduced here.

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman S. <ti...@im...>]

On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote:
> >> * Is it still correct nowadays that the function "gigaset_initcs" did not
> >>   call the function "kfree" after a later function call failed?
> > 
> > Yes, if it is handled in another place, Paul already did show you the place.
> 
> To which source code place do you refer here?

Obviously the one Paul pointed out to you in detail in his mail dated
Mon, 26 Sep 2016 23:13:54 +0200.

HTH,
Tilman

-- 
Tilman Schmidt
ti...@im...

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF M. E. <el...@us...>]

>> * Is it still correct nowadays that the function "gigaset_initcs" did not
>>   call the function "kfree" after a later function call failed?
> 
> Yes, if it is handled in another place, Paul already did show you the place.

To which source code place do you refer here?


>> * Do you expect that allocated memory will be automatically reclaimed
>>   after it would return a null pointer?
>>
> Of course not

Thanks for this acknowledgement.

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[<is...@li...>]

Am 27.09.2016 um 13:32 schrieb SF Markus Elfring:
>>> I got the impression that the exception handling  was incomplete in the
>>> implementation of the function "gigaset_initcs".
>>
>> That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?
> 

Yes, if it is handled in another place, Paul already did show you the place.


> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?
> 
Of course not

Best regards
Karsten

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman S. <ti...@im...>]

On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote:
> >> I got the impression that the exception handling  was incomplete in the
> >> implementation of the function "gigaset_initcs".
> > 
> > That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?

Wrong premise. That statement was never correct.

> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?

No. Should I? Do you?

Regards,
Tilman

-- 
Tilman Schmidt
ti...@im...