linux-kernel - Re: [PATCH net-next 2/2] eth: fbnic: Add devlink dev flash support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61e80187-49d0-4ad8-a66a-0c3901963201@trager.us>
Date: Fri, 18 Oct 2024 15:48:26 -0700
From: Lee Trager <lee@...ger.us>
To: Simon Horman <horms@...nel.org>,
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Alexander Duyck <alexanderduyck@...com>, Jakub Kicinski
 <kuba@...nel.org>, kernel-team@...a.com,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>,
 Mohsin Bashir <mohsin.bashr@...il.com>,
 Sanman Pradhan <sanmanpradhan@...a.com>, Al Viro <viro@...iv.linux.org.uk>,
 netdev@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] eth: fbnic: Add devlink dev flash support

On 10/15/24 3:43 AM, Simon Horman wrote:

> On Mon, Oct 14, 2024 at 12:18:36PM +0100, Vadim Fedorenko wrote:
>> On 12/10/2024 03:34, Lee Trager wrote:
>>> fbnic supports updating firmware using a PLDM image signed and distributed
>>> by Meta. PLDM images are written into stored flashed. Flashing does not
>>> interrupt operation.
>>>
>>> On host reboot the newly flashed UEFI driver will be used. To run new
>>> control or cmrt firmware the NIC must be power cycled.
>>>
>>> Signed-off-by: Lee Trager <lee@...ger.us>
> ...
>
>>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> ...
>
>>> +/**
>>> + * fbnic_send_component_table - Send PLDM component table to the firmware
>>> + * @context: PLDM FW update structure
>>> + * @component: The component to send
>>> + * @transfer_flag: Flag indication location in component tables
>>> + *
>>> + * Read relevant data from component table and forward it to the firmware.
>>> + * Check response to verify if the firmware indicates that it wishes to
>>> + * proceed with the update.
>>> + *
>>> + * Return: zero on success
>>> + *	    negative error code on failure
>>> + */
>>> +static int fbnic_send_component_table(struct pldmfw *context,
>>> +				      struct pldmfw_component *component,
>>> +				      u8 transfer_flag)
>>> +{
>>> +	struct device *dev = context->dev;
>>> +	u16 id = component->identifier;
>>> +	u8 test_string[80];
>>> +
>>> +	switch (id) {
>>> +	case QSPI_SECTION_CMRT:
>>> +	case QSPI_SECTION_CONTROL_FW:
>>> +	case QSPI_SECTION_OPTION_ROM:
>>> +		break;
>>> +	default:
>>> +		dev_err(dev, "Unknown component ID %u\n", id);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	dev_dbg(dev, "Sending PLDM component table to firmware\n");
>>> +
>>> +	/* Temp placeholder */
>>> +	memcpy(test_string, component->version_string,
>>> +	       min_t(u8, component->version_len, 79));
>>> +	test_string[min_t(u8, component->version_len, 79)] = 0;
>> Looks like this construction can be replaced with strscpy().
>> There were several patchsets in the tree to use strscpy(), let's follow
>> the pattern.
> While looking at these lines, I'm unsure why min_t() is being used
> instead of min() here. As version_len is unsigned and 79 is a positive
> constant, I believe min() should be fine here.

clang complains if I'm not explicit with the type by using min_t()


/home/ltrager/fbnic/src/fbnic_devlink.c:194:3: warning: comparison of 
distinct pointer types ('typeof (component->version_len) *' (aka 
'unsigned char *') and 'typeof (79) *' (aka 'int *')) 
[-Wcompare-distinct-pointer-types]

   194 | min(component->version_len, 79));

       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

./include/linux/minmax.h:45:19: note: expanded from macro 'min'

    45 | #define min(x, y) __careful_cmp(x, y, <)

       | ^~~~~~~~~~~~~~~~~~~~~~

./include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'

    36 | __builtin_choose_expr(__safe_cmp(x, y), \

       | ^~~~~~~~~~~~~~~~

./include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'

    26 |                 (__typecheck(x, y) && __no_side_effects(x, y))

       |                  ^~~~~~~~~~~~~~~~~

./include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'

    20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))

       |                    ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~

1 warning generated.

>>> +	dev_info(dev, "PLDMFW: Component ID: %u version %s\n",
>>> +		 id, test_string);
>>> +
>>> +	return 0;
>>> +}