Skip to contentSkip to conversation

mfd: 64 bit bug in bxtwc_val_store()

PATCH Applied Dan Carpenter 2 participants 6 messages Sep 28, 2015 – Oct 1, 2015

Subsystems (3)
Kernel janitors Multifunction devices (MFD) Intel PMIC multifunction device drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 mfd: 64 bit bug in bxtwc_val_store()

The call to kstrtoul() will corrupt memory on 64 bit systems because an int is 4 bytes and a long is 8.

Also it's not a good idea to let users trigger a dev_err() because it just ends up flooding /var/log/messages so I removed the printk.

Fixes 2ddd2086ea9c ('mfd: add Intel Broxton Whiskey Cove PMIC driver')
Signed-off-by Dan Carpenter <[email protected]>
drivers/mfd/intel_soc_pmic_bxtwc.c +3 -4
index 40acaff..b942876 100644
@@ -297,10 +297,9 @@ static ssize_t bxtwc_val_store(struct device *dev,
 	unsigned int val;
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
-	if (kstrtoul(buf, 0, (unsigned long *)&val)) {
-		dev_err(dev, "Invalid register value\n");
-		return -EINVAL;
-	}
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	ret = regmap_write(pmic->regmap, bxtwc_reg_addr, val);
 	if (ret) {
Applied
#1 lore raw

I will fix the subject line for this patch, but in future please use the format akin to the individual subsystem.

In the MFD case this is:

mfd: <device>: <Summary starting with an uppercase character>

Quoted 8 lines

The call to kstrtoul() will corrupt memory on 64 bit systems because an int is 4 bytes and a long is 8.

Also it's not a good idea to let users trigger a dev_err() because it just ends up flooding /var/log/messages so I removed the printk.

Fixes 2ddd2086ea9c ('mfd: add Intel Broxton Whiskey Cove PMIC driver')
Signed-off-by Dan Carpenter <[email protected]>

Applied, thanks.

Quoted 18 lines
drivers/mfd/intel_soc_pmic_bxtwc.c +3 -4
index 40acaff..b942876 100644
@@ -297,10 +297,9 @@ static ssize_t bxtwc_val_store(struct device *dev,
 	unsigned int val;
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
-	if (kstrtoul(buf, 0, (unsigned long *)&val)) {
-		dev_err(dev, "Invalid register value\n");
-		return -EINVAL;
-	}
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	ret = regmap_write(pmic->regmap, bxtwc_reg_addr, val);
 	if (ret) {
Signature · 8 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#2 lore raw
In reply to Dan Carpenter
Quoted 7 lines · Lee Jones

I will fix the subject line for this patch, but in future please use the format akin to the individual subsystem.

In the MFD case this is:

mfd: <device>: <Summary starting with an uppercase character>

I looked at the subjects to every other patch to that file and I followed the same pattern.

$ git log --oneline drivers/mfd/intel_soc_pmic_bxtwc.c
b412836 mfd: add Intel Broxton Whiskey Cove PMIC driver
$

regards,
dan carpenter

author
#3 lore raw
In reply to Dan Carpenter
Quoted 2 lines · Dan Carpenter

I looked at the subjects to every other patch to that file and I followed the same pattern.

This is literally a part of my kpatch.sh script:

git log --oneline $fullname | head -n 10
echo "Copy and paste one of these subjects?"
read unused

regards,
dan carpenter

#4 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter
Quoted 2 lines · Dan Carpenter

I looked at the subjects to every other patch to that file and I followed the same pattern.

This is literally a part of my kpatch.sh script:

git log --oneline $fullname | head -n 10
echo "Copy and paste one of these subjects?"
read unused

Okay, this is good. Unfortunately I guess process over-ruled common sense on this occasion (meant more of a joke than offensively). ;)

When new drivers are sent, the patch format is more likely to be:

mfd: "Adding new driver for ..."

As there is no <device> to mention. However, as yours is an add-on patch which makes changes to an existing driver, a better subject line would have been:

mfd: <device>: "I'm doing this"

... but I completely understand the confusion. Not much to change then, keep doing what you're doing.

Signature · 8 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#5 lore raw
In reply to Dan Carpenter

I'm often the first person to touch a new patch after the author so this is a conversation I have had before. I "persuaded" the drivers/media people and now they write patch descriptions like:

85756a0 [media] cobalt: add new driver

regards,
dan carpenter