* [PATCH] hwmon: Remove strlcpy occurences
@ 2023-07-12 14:44 Azeem Shaikh
2023-07-12 15:16 ` Guenter Roeck
2023-07-12 15:39 ` Guenter Roeck
0 siblings, 2 replies; 5+ messages in thread
From: Azeem Shaikh @ 2023-07-12 14:44 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-hardening, Azeem Shaikh, linux-hwmon, linux-kernel,
Jean Delvare, Kees Cook
strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with direct assignment.
strlcpy in this file is used to copy fixed-length strings which can be
completely avoided by direct assignment and is safe to do so. strlen()
is used to return the length of @tbuf.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89
Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
drivers/hwmon/pmbus/max20730.c | 64 +++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 31 deletions(-)
diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
index 7bcf27995033..f5ba23f0fed5 100644
--- a/drivers/hwmon/pmbus/max20730.c
+++ b/drivers/hwmon/pmbus/max20730.c
@@ -113,7 +113,8 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
struct max20730_debugfs_data *psu = to_psu(idxp, idx);
const struct pmbus_driver_info *info;
const struct max20730_data *data;
- char tbuf[DEBUG_FS_DATA_MAX] = { 0 };
+ char tbuf[DEBUG_FS_DATA_MAX] = {};
+ char *result = tbuf;
u16 val;
info = pmbus_get_driver_info(psu->client);
@@ -148,13 +149,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>> MAX20730_MFR_DEVSET1_TSTAT_BIT_POS;
if (val == 0)
- len = strlcpy(tbuf, "2000\n", DEBUG_FS_DATA_MAX);
+ result = "2000\n";
else if (val == 1)
- len = strlcpy(tbuf, "125\n", DEBUG_FS_DATA_MAX);
+ result = "125\n";
else if (val == 2)
- len = strlcpy(tbuf, "62.5\n", DEBUG_FS_DATA_MAX);
+ result = "62.5\n";
else
- len = strlcpy(tbuf, "32\n", DEBUG_FS_DATA_MAX);
+ result = "32\n";
break;
case MAX20730_DEBUGFS_INTERNAL_GAIN:
val = (data->mfr_devset1 & MAX20730_MFR_DEVSET1_RGAIN_MASK)
@@ -163,35 +164,35 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
if (data->id == max20734) {
/* AN6209 */
if (val == 0)
- len = strlcpy(tbuf, "0.8\n", DEBUG_FS_DATA_MAX);
+ result = "0.8\n";
else if (val == 1)
- len = strlcpy(tbuf, "3.2\n", DEBUG_FS_DATA_MAX);
+ result = "3.2\n";
else if (val == 2)
- len = strlcpy(tbuf, "1.6\n", DEBUG_FS_DATA_MAX);
+ result = "1.6\n";
else
- len = strlcpy(tbuf, "6.4\n", DEBUG_FS_DATA_MAX);
+ result = "6.4\n";
} else if (data->id == max20730 || data->id == max20710) {
/* AN6042 or AN6140 */
if (val == 0)
- len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
+ result = "0.9\n";
else if (val == 1)
- len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
+ result = "3.6\n";
else if (val == 2)
- len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
+ result = "1.8\n";
else
- len = strlcpy(tbuf, "7.2\n", DEBUG_FS_DATA_MAX);
+ result = "7.2\n";
} else if (data->id == max20743) {
/* AN6042 */
if (val == 0)
- len = strlcpy(tbuf, "0.45\n", DEBUG_FS_DATA_MAX);
+ result = "0.45\n";
else if (val == 1)
- len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
+ result = "1.8\n";
else if (val == 2)
- len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
+ result = "0.9\n";
else
- len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
+ result = "3.6\n";
} else {
- len = strlcpy(tbuf, "Not supported\n", DEBUG_FS_DATA_MAX);
+ result = "Not supported\n";
}
break;
case MAX20730_DEBUGFS_BOOT_VOLTAGE:
@@ -199,26 +200,26 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>> MAX20730_MFR_DEVSET1_VBOOT_BIT_POS;
if (val == 0)
- len = strlcpy(tbuf, "0.6484\n", DEBUG_FS_DATA_MAX);
+ result = "0.6484\n";
else if (val == 1)
- len = strlcpy(tbuf, "0.8984\n", DEBUG_FS_DATA_MAX);
+ result = "0.8984\n";
else if (val == 2)
- len = strlcpy(tbuf, "1.0\n", DEBUG_FS_DATA_MAX);
+ result = "1.0\n";
else
- len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
+ result = "Invalid\n";
break;
case MAX20730_DEBUGFS_OUT_V_RAMP_RATE:
val = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_VRATE)
>> MAX20730_MFR_DEVSET2_VRATE_BIT_POS;
if (val == 0)
- len = strlcpy(tbuf, "4\n", DEBUG_FS_DATA_MAX);
+ result = "4\n";
else if (val == 1)
- len = strlcpy(tbuf, "2\n", DEBUG_FS_DATA_MAX);
+ result = "2\n";
else if (val == 2)
- len = strlcpy(tbuf, "1\n", DEBUG_FS_DATA_MAX);
+ result = "1\n";
else
- len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
+ result = "Invalid\n";
break;
case MAX20730_DEBUGFS_OC_PROTECT_MODE:
ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_OCPM_MASK)
@@ -230,13 +231,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
>> MAX20730_MFR_DEVSET2_SS_BIT_POS;
if (val == 0)
- len = strlcpy(tbuf, "0.75\n", DEBUG_FS_DATA_MAX);
+ result = "0.75\n";
else if (val == 1)
- len = strlcpy(tbuf, "1.5\n", DEBUG_FS_DATA_MAX);
+ result = "1.5\n";
else if (val == 2)
- len = strlcpy(tbuf, "3\n", DEBUG_FS_DATA_MAX);
+ result = "3\n";
else
- len = strlcpy(tbuf, "6\n", DEBUG_FS_DATA_MAX);
+ result = "6\n";
break;
case MAX20730_DEBUGFS_IMAX:
ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_IMAX_MASK)
@@ -287,9 +288,10 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
"%d.%d\n", ret / 10000, ret % 10000);
break;
default:
- len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
+ result = "Invalid\n";
}
+ len = strlen(result);
return simple_read_from_buffer(buf, count, ppos, tbuf, len);
}
--
2.41.0.255.g8b1d071c50-goog
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hwmon: Remove strlcpy occurences
2023-07-12 14:44 [PATCH] hwmon: Remove strlcpy occurences Azeem Shaikh
@ 2023-07-12 15:16 ` Guenter Roeck
2023-07-12 18:04 ` Kees Cook
2023-07-12 15:39 ` Guenter Roeck
1 sibling, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2023-07-12 15:16 UTC (permalink / raw)
To: Azeem Shaikh
Cc: linux-hardening, linux-hwmon, linux-kernel, Jean Delvare, Kees Cook
On 7/12/23 07:44, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with direct assignment.
>
> strlcpy in this file is used to copy fixed-length strings which can be
> completely avoided by direct assignment and is safe to do so. strlen()
> is used to return the length of @tbuf.
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
>
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
> drivers/hwmon/pmbus/max20730.c | 64 +++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
> index 7bcf27995033..f5ba23f0fed5 100644
> --- a/drivers/hwmon/pmbus/max20730.c
> +++ b/drivers/hwmon/pmbus/max20730.c
> @@ -113,7 +113,8 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
> struct max20730_debugfs_data *psu = to_psu(idxp, idx);
> const struct pmbus_driver_info *info;
> const struct max20730_data *data;
> - char tbuf[DEBUG_FS_DATA_MAX] = { 0 };
> + char tbuf[DEBUG_FS_DATA_MAX] = {};
Unrelated change.
> + char *result = tbuf;
> u16 val;
>
> info = pmbus_get_driver_info(psu->client);
> @@ -148,13 +149,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
> >> MAX20730_MFR_DEVSET1_TSTAT_BIT_POS;
>
> if (val == 0)
> - len = strlcpy(tbuf, "2000\n", DEBUG_FS_DATA_MAX);
> + result = "2000\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "125\n", DEBUG_FS_DATA_MAX);
> + result = "125\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "62.5\n", DEBUG_FS_DATA_MAX);
> + result = "62.5\n";
> else
> - len = strlcpy(tbuf, "32\n", DEBUG_FS_DATA_MAX);
> + result = "32\n";
> break;
> case MAX20730_DEBUGFS_INTERNAL_GAIN:
> val = (data->mfr_devset1 & MAX20730_MFR_DEVSET1_RGAIN_MASK)
> @@ -163,35 +164,35 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
> if (data->id == max20734) {
> /* AN6209 */
> if (val == 0)
> - len = strlcpy(tbuf, "0.8\n", DEBUG_FS_DATA_MAX);
> + result = "0.8\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "3.2\n", DEBUG_FS_DATA_MAX);
> + result = "3.2\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "1.6\n", DEBUG_FS_DATA_MAX);
> + result = "1.6\n";
> else
> - len = strlcpy(tbuf, "6.4\n", DEBUG_FS_DATA_MAX);
> + result = "6.4\n";
> } else if (data->id == max20730 || data->id == max20710) {
> /* AN6042 or AN6140 */
> if (val == 0)
> - len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
> + result = "0.9\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
> + result = "3.6\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
> + result = "1.8\n";
> else
> - len = strlcpy(tbuf, "7.2\n", DEBUG_FS_DATA_MAX);
> + result = "7.2\n";
> } else if (data->id == max20743) {
> /* AN6042 */
> if (val == 0)
> - len = strlcpy(tbuf, "0.45\n", DEBUG_FS_DATA_MAX);
> + result = "0.45\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "1.8\n", DEBUG_FS_DATA_MAX);
> + result = "1.8\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "0.9\n", DEBUG_FS_DATA_MAX);
> + result = "0.9\n";
> else
> - len = strlcpy(tbuf, "3.6\n", DEBUG_FS_DATA_MAX);
> + result = "3.6\n";
> } else {
> - len = strlcpy(tbuf, "Not supported\n", DEBUG_FS_DATA_MAX);
> + result = "Not supported\n";
> }
> break;
> case MAX20730_DEBUGFS_BOOT_VOLTAGE:
> @@ -199,26 +200,26 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
> >> MAX20730_MFR_DEVSET1_VBOOT_BIT_POS;
>
> if (val == 0)
> - len = strlcpy(tbuf, "0.6484\n", DEBUG_FS_DATA_MAX);
> + result = "0.6484\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "0.8984\n", DEBUG_FS_DATA_MAX);
> + result = "0.8984\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "1.0\n", DEBUG_FS_DATA_MAX);
> + result = "1.0\n";
> else
> - len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
> + result = "Invalid\n";
> break;
> case MAX20730_DEBUGFS_OUT_V_RAMP_RATE:
> val = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_VRATE)
> >> MAX20730_MFR_DEVSET2_VRATE_BIT_POS;
>
> if (val == 0)
> - len = strlcpy(tbuf, "4\n", DEBUG_FS_DATA_MAX);
> + result = "4\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "2\n", DEBUG_FS_DATA_MAX);
> + result = "2\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "1\n", DEBUG_FS_DATA_MAX);
> + result = "1\n";
> else
> - len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
> + result = "Invalid\n";
> break;
> case MAX20730_DEBUGFS_OC_PROTECT_MODE:
> ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_OCPM_MASK)
> @@ -230,13 +231,13 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
> >> MAX20730_MFR_DEVSET2_SS_BIT_POS;
>
> if (val == 0)
> - len = strlcpy(tbuf, "0.75\n", DEBUG_FS_DATA_MAX);
> + result = "0.75\n";
> else if (val == 1)
> - len = strlcpy(tbuf, "1.5\n", DEBUG_FS_DATA_MAX);
> + result = "1.5\n";
> else if (val == 2)
> - len = strlcpy(tbuf, "3\n", DEBUG_FS_DATA_MAX);
> + result = "3\n";
> else
> - len = strlcpy(tbuf, "6\n", DEBUG_FS_DATA_MAX);
> + result = "6\n";
> break;
> case MAX20730_DEBUGFS_IMAX:
> ret = (data->mfr_devset2 & MAX20730_MFR_DEVSET2_IMAX_MASK)
> @@ -287,9 +288,10 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf,
> "%d.%d\n", ret / 10000, ret % 10000);
> break;
> default:
> - len = strlcpy(tbuf, "Invalid\n", DEBUG_FS_DATA_MAX);
> + result = "Invalid\n";
> }
>
> + len = strlen(result);
> return simple_read_from_buffer(buf, count, ppos, tbuf, len);
This still copies tbuf, meaning all those constant strings won't actually
be reported.
> }
>
> --
> 2.41.0.255.g8b1d071c50-goog
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hwmon: Remove strlcpy occurences
2023-07-12 15:16 ` Guenter Roeck
@ 2023-07-12 18:04 ` Kees Cook
2023-07-12 19:25 ` Azeem Shaikh
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2023-07-12 18:04 UTC (permalink / raw)
To: Guenter Roeck
Cc: Azeem Shaikh, linux-hardening, linux-hwmon, linux-kernel, Jean Delvare
On Wed, Jul 12, 2023 at 08:16:39AM -0700, Guenter Roeck wrote:
> On 7/12/23 07:44, Azeem Shaikh wrote:
> > [...]
> > + len = strlen(result);
> > return simple_read_from_buffer(buf, count, ppos, tbuf, len);
>
> This still copies tbuf, meaning all those constant strings won't actually
> be reported.
Ah dang, thanks for catching that. Yeah, "tbuf" should be replaced by
"result" in the simple_read_from_buffer().
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: Remove strlcpy occurences
2023-07-12 18:04 ` Kees Cook
@ 2023-07-12 19:25 ` Azeem Shaikh
0 siblings, 0 replies; 5+ messages in thread
From: Azeem Shaikh @ 2023-07-12 19:25 UTC (permalink / raw)
To: Kees Cook
Cc: Guenter Roeck, linux-hardening, linux-hwmon, linux-kernel, Jean Delvare
On Wed, Jul 12, 2023 at 2:04 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jul 12, 2023 at 08:16:39AM -0700, Guenter Roeck wrote:
> > On 7/12/23 07:44, Azeem Shaikh wrote:
> > > [...]
> > > + len = strlen(result);
> > > return simple_read_from_buffer(buf, count, ppos, tbuf, len);
> >
> > This still copies tbuf, meaning all those constant strings won't actually
> > be reported.
>
> Ah dang, thanks for catching that. Yeah, "tbuf" should be replaced by
> "result" in the simple_read_from_buffer().
>
Thanks for review folks, will send out a v2 shortly.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: Remove strlcpy occurences
2023-07-12 14:44 [PATCH] hwmon: Remove strlcpy occurences Azeem Shaikh
2023-07-12 15:16 ` Guenter Roeck
@ 2023-07-12 15:39 ` Guenter Roeck
1 sibling, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2023-07-12 15:39 UTC (permalink / raw)
To: Azeem Shaikh
Cc: linux-hardening, linux-hwmon, linux-kernel, Jean Delvare, Kees Cook
On 7/12/23 07:44, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with direct assignment.
>
> strlcpy in this file is used to copy fixed-length strings which can be
> completely avoided by direct assignment and is safe to do so. strlen()
> is used to return the length of @tbuf.
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
>
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
Also, $subject should include the affected driver.
Guenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-12 19:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-12 14:44 [PATCH] hwmon: Remove strlcpy occurences Azeem Shaikh
2023-07-12 15:16 ` Guenter Roeck
2023-07-12 18:04 ` Kees Cook
2023-07-12 19:25 ` Azeem Shaikh
2023-07-12 15:39 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®