On Mon, Dec 11, 2023 at 08:06:28PM +0000, Justin Stitt wrote: > Instead of copying @buf into a new buffer and carefully managing its > newline/null-terminating status, we can just use sysfs_match_string() > as it uses sysfs_streq() internally which handles newline/null-term: > > | /** > | * sysfs_streq - return true if strings are equal, modulo trailing newline > | * @s1: one string > | * @s2: another string > | * > | * This routine returns true iff two strings are equal, treating both > | * NUL and newline-then-NUL as equivalent string terminations. It's > | * geared for use with sysfs input strings, which generally terminate > | * with newlines but are compared against values without newlines. > | */ > | bool sysfs_streq(const char *s1, const char *s2) > | ... > > Then entirely drop the now unused fcoe_parse_mode, being careful to > change if condition from checking for FIP_CONN_TYPE_UNKNOWN to < 0 as > sysfs_match_string can return -EINVAL. > > To get the compiler not to complain, make fip_conn_type_names > const char * const. Perhaps, this should also be done for > fcf_state_names. > > This also removes an instance of strncpy() which helps [1]. > > Link: https://github.com/KSPP/linux/issues/90 [1] > Cc: linux-hardening@xxxxxxxxxxxxxxx > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > --- > Builds upon patch and feedback from [2]: > > However, this is different enough to warrant its own patch and not be a > continuation. > > [2]: https://lore.kernel.org/all/9f38f4aa-c6b5-4786-a641-d02d8bd92f7f@xxxxxxx/ > --- > drivers/scsi/fcoe/fcoe_sysfs.c | 26 ++++---------------------- > 1 file changed, 4 insertions(+), 22 deletions(-) My favorite kind of insert/delete ratio! :) > > diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c > index e17957f8085c..f9c5d00f658a 100644 > --- a/drivers/scsi/fcoe/fcoe_sysfs.c > +++ b/drivers/scsi/fcoe/fcoe_sysfs.c > @@ -10,6 +10,7 @@ > #include <linux/kernel.h> > #include <linux/etherdevice.h> > #include <linux/ctype.h> > +#include <linux/string.h> > > #include <scsi/fcoe_sysfs.h> > #include <scsi/libfcoe.h> > @@ -214,25 +215,13 @@ static const char *get_fcoe_##title##_name(enum table_type table_key) \ > return table[table_key]; \ > } > > -static char *fip_conn_type_names[] = { > +static const char * const fip_conn_type_names[] = { > [ FIP_CONN_TYPE_UNKNOWN ] = "Unknown", > [ FIP_CONN_TYPE_FABRIC ] = "Fabric", > [ FIP_CONN_TYPE_VN2VN ] = "VN2VN", > }; > fcoe_enum_name_search(ctlr_mode, fip_conn_type, fip_conn_type_names) > > -static enum fip_conn_type fcoe_parse_mode(const char *buf) > -{ > - int i; > - > - for (i = 0; i < ARRAY_SIZE(fip_conn_type_names); i++) { > - if (strcasecmp(buf, fip_conn_type_names[i]) == 0) > - return i; > - } > - > - return FIP_CONN_TYPE_UNKNOWN; > -} > - > static char *fcf_state_names[] = { > [ FCOE_FCF_STATE_UNKNOWN ] = "Unknown", > [ FCOE_FCF_STATE_DISCONNECTED ] = "Disconnected", > @@ -274,17 +263,10 @@ static ssize_t store_ctlr_mode(struct device *dev, > const char *buf, size_t count) > { > struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev); > - char mode[FCOE_MAX_MODENAME_LEN + 1]; > > if (count > FCOE_MAX_MODENAME_LEN) > return -EINVAL; > > - strncpy(mode, buf, count); > - > - if (mode[count - 1] == '\n') > - mode[count - 1] = '\0'; > - else > - mode[count] = '\0'; > > switch (ctlr->enabled) { > case FCOE_CTLR_ENABLED: > @@ -297,8 +279,8 @@ static ssize_t store_ctlr_mode(struct device *dev, > return -ENOTSUPP; > } > > - ctlr->mode = fcoe_parse_mode(mode); > - if (ctlr->mode == FIP_CONN_TYPE_UNKNOWN) { > + ctlr->mode = sysfs_match_string(fip_conn_type_names, buf); > + if (ctlr->mode < 0) { I think this needs to include FIP_CONN_TYPE_UNKNOWN to keep the logic the same? (i.e. it could match the string "Unknown", so it would return the enum value for that, 0 in this case.) Otherwise, yeah, this looks good. -Kees > LIBFCOE_SYSFS_DBG(ctlr, "Unknown mode %s provided.\n", > buf); > return -EINVAL; > > --- > base-commit: bee0e7762ad2c6025b9f5245c040fcc36ef2bde8 > change-id: 20231024-strncpy-drivers-scsi-fcoe-fcoe_sysfs-c-0e1dffe82855 > > Best regards, > -- > Justin Stitt <justinstitt@xxxxxxxxxx> > > -- Kees Cook
