Fix incorrect usage of strncpy and strncat in i8042_pnp_kbd_probe(); drivers/input/serio/i8042-x86ia64io.h

PATCH In review Roel Kluin 2 participants 3 messages Nov 5, 2007

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Roel Kluin

author

2007-11-05 20:24 UTC #0 lore raw

PATCH 1/1 Fix incorrect usage of strncpy and strncat in i8042_pnp_kbd_probe(); drivers/input/serio/i8042-x86ia64io.h

See http://www.gratisoft.us/todd/papers/strlcpy.html
--
Fix incorrect length argument for strncpy and strncat by replacing them with respectively strlcpy and strlcat

Signed-off-by Roel Kluin <[email protected]>

---

drivers/input/serio/i8042-x86ia64io.h +21 -6

index f8fe421..4e6bd5f 100644
@@ -288,6 +288,8 @@ static char i8042_pnp_aux_name[32];
 
 static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *did)
 {
+	const unsigned int nsize = sizeof(i8042_pnp_kbd_name);
+
 	if (pnp_port_valid(dev, 0) && pnp_port_len(dev, 0) == 1)
 		i8042_pnp_data_reg = pnp_port_start(dev,0);

@@ -297,10 +299,16 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
 	if (pnp_irq_valid(dev,0))
 		i8042_pnp_kbd_irq = pnp_irq(dev, 0);
 
-	strncpy(i8042_pnp_kbd_name, did->id, sizeof(i8042_pnp_kbd_name));
+	if (strlcpy(i8042_pnp_kbd_name, did->id, nsize) >= nsize)
+		return -ENAMETOOLONG;
+
 	if (strlen(pnp_dev_name(dev))) {
-		strncat(i8042_pnp_kbd_name, ":", sizeof(i8042_pnp_kbd_name));
-		strncat(i8042_pnp_kbd_name, pnp_dev_name(dev), sizeof(i8042_pnp_kbd_name));
+		if (strlcat(i8042_pnp_kbd_name, ":", nsize) >= nsize)
+			return -ENAMETOOLONG;
+
+		if (strlcat(i8042_pnp_kbd_name, pnp_dev_name(dev), nsize) >= nsize)
+			return -ENAMETOOLONG;
+
 	}
 
 	i8042_pnp_kbd_devices++;

@@ -309,6 +317,8 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
 
 static int i8042_pnp_aux_probe(struct pnp_dev *dev, const struct pnp_device_id *did)
 {
+	const unsigned int nsize = sizeof(i8042_pnp_aux_name);
+
 	if (pnp_port_valid(dev, 0) && pnp_port_len(dev, 0) == 1)
 		i8042_pnp_data_reg = pnp_port_start(dev,0);

@@ -318,10 +328,15 @@ static int i8042_pnp_aux_probe(struct pnp_dev *dev, const struct pnp_device_id *
 	if (pnp_irq_valid(dev, 0))
 		i8042_pnp_aux_irq = pnp_irq(dev, 0);
 
-	strncpy(i8042_pnp_aux_name, did->id, sizeof(i8042_pnp_aux_name));
+	if (strlcpy(i8042_pnp_aux_name, did->id, nsize) >= nsize)
+		return -ENAMETOOLONG;
+
 	if (strlen(pnp_dev_name(dev))) {
-		strncat(i8042_pnp_aux_name, ":", sizeof(i8042_pnp_aux_name));
-		strncat(i8042_pnp_aux_name, pnp_dev_name(dev), sizeof(i8042_pnp_aux_name));
+		if (strlcat(i8042_pnp_aux_name, ":", nsize) >= nsize)
+			return -ENAMETOOLONG;
+
+		if (strlcat(i8042_pnp_aux_name, pnp_dev_name(dev), nsize) >= nsize)
+			return -ENAMETOOLONG;
 	}
 
 	i8042_pnp_aux_devices++;

Dmitry Torokhov

maintainer

2007-11-05 21:08 UTC #1 lore raw

Hi Roel,

Quoted 3 lines · Roel Kluin

See http://www.gratisoft.us/todd/papers/strlcpy.html
--
Fix incorrect length argument for strncpy and strncat by replacing them with

What is incorrect about the argument?

Quoted 1 line

respectively strlcpy and strlcat

I will gladly take a patch replacing strncat with strlcat but don't error out if the buffer is too small - it is oK if PNP name is printed truncated, it does not affect KBC operations.

Thanks.

Signature · 1 line

Dmitry

Roel Kluin

author

2007-11-05 22:30 UTC #2 lore raw

Quoted 8 lines · Dmitry Torokhov

Hi Roel,
Quoted 3 lines · Roel Kluin
See http://www.gratisoft.us/todd/papers/strlcpy.html
--
Fix incorrect length argument for strncpy and strncat by replacing them with
What is incorrect about the argument?

As explained in the article, strncpy and strncat should not get sizeof(target) as a third argument, but rather, for strncpy 'sizeof(target) -1' and for strncat 'sizeof(target) - strlen(target) - 1'.

for strlcpy and strlcat sizeof(target) should be used. Also strncpy and strncat zero out the remainder of the characters - if source is shorter than target - which makes it slower than the strl- variants.

Quoted 3 lines

I will gladly take a patch replacing strncat with strlcat but don't error out if the buffer is too small - it is oK if PNP name is printed truncated, it does not affect KBC operations.

Ok, updated patch below.

--
Fix incorrect length argument for strncpy and strncat by replacing them with strlcpy and strlcat

Signed-off-by Roel Kluin <[email protected]>

---

drivers/input/serio/i8042-x86ia64io.h +6 -6

index f8fe421..1a739cf 100644
@@ -297,10 +297,10 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
 	if (pnp_irq_valid(dev,0))
 		i8042_pnp_kbd_irq = pnp_irq(dev, 0);
 
-	strncpy(i8042_pnp_kbd_name, did->id, sizeof(i8042_pnp_kbd_name));
+	strlcpy(i8042_pnp_kbd_name, did->id, sizeof(i8042_pnp_kbd_name));
 	if (strlen(pnp_dev_name(dev))) {
-		strncat(i8042_pnp_kbd_name, ":", sizeof(i8042_pnp_kbd_name));
-		strncat(i8042_pnp_kbd_name, pnp_dev_name(dev), sizeof(i8042_pnp_kbd_name));
+		strlcat(i8042_pnp_kbd_name, ":", sizeof(i8042_pnp_kbd_name));
+		strlcat(i8042_pnp_kbd_name, pnp_dev_name(dev), sizeof(i8042_pnp_kbd_name));
 	}
 
 	i8042_pnp_kbd_devices++;

@@ -318,10 +318,10 @@ static int i8042_pnp_aux_probe(struct pnp_dev *dev, const struct pnp_device_id *
 	if (pnp_irq_valid(dev, 0))
 		i8042_pnp_aux_irq = pnp_irq(dev, 0);
 
-	strncpy(i8042_pnp_aux_name, did->id, sizeof(i8042_pnp_aux_name));
+	strlcpy(i8042_pnp_aux_name, did->id, sizeof(i8042_pnp_aux_name));
 	if (strlen(pnp_dev_name(dev))) {
-		strncat(i8042_pnp_aux_name, ":", sizeof(i8042_pnp_aux_name));
-		strncat(i8042_pnp_aux_name, pnp_dev_name(dev), sizeof(i8042_pnp_aux_name));
+		strlcat(i8042_pnp_aux_name, ":", sizeof(i8042_pnp_aux_name));
+		strlcat(i8042_pnp_aux_name, pnp_dev_name(dev), sizeof(i8042_pnp_aux_name));
 	}
 
 	i8042_pnp_aux_devices++;