On Wed, Jul 12, 2023 at 09:43:07PM +0000, 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@xxxxxxxxx> > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> Applied. Thanks, Guenter > --- > v2: > * Includes driver name in commit header. > * Removes unrelated change tbuf{ 0 } -> tbuf{}. > * Fixes bug where @tbuf was being passed instead of @result. > > v1: > * https://lore.kernel.org/all/20230712144429.2845940-1-azeemshaikh38@xxxxxxxxx/ > > drivers/hwmon/pmbus/max20730.c | 64 +++++++++++++++++++++-------------------- > 1 file changed, 33 insertions(+), 31 deletions(-) > > -- > 2.41.0.255.g8b1d071c50-goog > > diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c > index 7bcf27995033..1de34da177c9 100644 > --- a/drivers/hwmon/pmbus/max20730.c > +++ b/drivers/hwmon/pmbus/max20730.c > @@ -114,6 +114,7 @@ static ssize_t max20730_debugfs_read(struct file *file, char __user *buf, > const struct pmbus_driver_info *info; > const struct max20730_data *data; > char tbuf[DEBUG_FS_DATA_MAX] = { 0 }; > + 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,10 +288,11 @@ 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"; > } > > - return simple_read_from_buffer(buf, count, ppos, tbuf, len); > + len = strlen(result); > + return simple_read_from_buffer(buf, count, ppos, result, len); > } > > static const struct file_operations max20730_fops = {
