| 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
| ||
|
Message-ID: <828b4e9a-9e1f-a643-7fed-d4d33e8d8d8e@amd.com>
Date: Fri, 17 Feb 2023 07:49:11 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: Andreas Noever <andreas.noever@...il.com>,
Michael Jamet <michael.jamet@...el.com>,
Yehezkel Bernat <YehezkelShB@...il.com>, Sanju.Mehta@....com,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] thunderbolt: Refactor DROM reading
On 2/17/23 07:36, Mika Westerberg wrote:
> Hi Mario,
>
> On Thu, Feb 16, 2023 at 02:19:10PM -0600, Mario Limonciello wrote:
>> The NVM reading code has a series of gotos that potentially introduce
>> unexpected behaviors with retries if something unexpected has failed
>> to parse.
>>
>> Additionally the retry logic introduced in commit f022ff7bf377
>> ("thunderbolt: Retry DROM read once if parsing fails") was added from
>> failures in bit banging, which aren't expected to be present when the
>> DROM is fetched directly from the NVM.
> Okay that's why you remove the EILSEQ returns below, right?
Yep, that's right.
>> Refactor the code to remove the gotos and drop the retry logic.
> Thanks for doing this.
Sure.
> Few minor stylistic comments below. I can also
> fix these myself when applying if you prefer.
If there is any other feedback on the other patches in the series from
you or anyone else before 6.3-rc1 I'll send out a v4 fixup the style
stuff outlined below.
Otherwise, I'll take up your offer thanks for that.
> Note I will be on vacation next week but will be picking up patches once
> v6.3-rc1 is released.
OK, appreciated.
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> v2->v3:
>> * Split out refactor
>> ---
>> drivers/thunderbolt/eeprom.c | 147 +++++++++++++++++++----------------
>> 1 file changed, 79 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
>> index a326cf16ca3d..2a078c69f0d2 100644
>> --- a/drivers/thunderbolt/eeprom.c
>> +++ b/drivers/thunderbolt/eeprom.c
>> @@ -416,7 +416,7 @@ static int tb_drom_parse_entries(struct tb_switch *sw, size_t header_size)
>> if (pos + 1 == drom_size || pos + entry->len > drom_size
>> || !entry->len) {
>> tb_sw_warn(sw, "DROM buffer overrun\n");
>> - return -EILSEQ;
>> + return -EIO;
>> }
>>
>> switch (entry->type) {
>> @@ -543,7 +543,37 @@ static int tb_drom_read_n(struct tb_switch *sw, u16 offset, u8 *val,
>> return tb_eeprom_read_n(sw, offset, val, count);
>> }
>>
>> -static int tb_drom_parse(struct tb_switch *sw)
>> +static int tb_drom_bit_bang(struct tb_switch *sw, u16 *size)
>> +{
>> + int res;
> ret
>
>> +
>> + res = tb_drom_read_n(sw, 14, (u8 *) size, 2);
>> + if (res)
>> + return res;
> empty line here.
>
>> + *size &= 0x3ff;
>> + *size += TB_DROM_DATA_START;
> here too.
>
>> + tb_sw_dbg(sw, "reading drom (length: %#x)\n", *size);
>> + if (*size < sizeof(struct tb_drom_header)) {
>> + tb_sw_warn(sw, "drom too small, aborting\n");
> DROM
>
>> + return -EIO;
>> + }
>> +
>> + sw->drom = kzalloc(*size, GFP_KERNEL);
>> + if (!sw->drom)
>> + return -ENOMEM;
>> +
>> + res = tb_drom_read_n(sw, 0, sw->drom, *size);
>> + if (res)
>> + goto err;
>> +
>> + return 0;
> empty line
>
>> +err:
>> + kfree(sw->drom);
>> + sw->drom = NULL;
> empty line
>
>> + return res;
>> +}
>> +
>> +static int tb_drom_parse_v1(struct tb_switch *sw)
>> {
>> const struct tb_drom_header *header =
>> (const struct tb_drom_header *)sw->drom;
>> @@ -554,7 +584,7 @@ static int tb_drom_parse(struct tb_switch *sw)
>> tb_sw_warn(sw,
>> "DROM UID CRC8 mismatch (expected: %#x, got: %#x)\n",
>> header->uid_crc8, crc);
>> - return -EILSEQ;
>> + return -EIO;
>> }
>> if (!sw->uid)
>> sw->uid = header->uid;
>> @@ -588,6 +618,43 @@ static int usb4_drom_parse(struct tb_switch *sw)
>> return tb_drom_parse_entries(sw, USB4_DROM_HEADER_SIZE);
>> }
>>
>> +static int tb_drom_parse(struct tb_switch *sw, u16 *size)
>> +{
>> + struct tb_drom_header *header = (void *) sw->drom;
>> + int res;
> ret
>
>> +
>> + if (header->data_len + TB_DROM_DATA_START != *size) {
>> + tb_sw_warn(sw, "drom size mismatch\n");
> DROM
>
>> + goto err;
>> + }
>> +
>> + tb_sw_dbg(sw, "DROM version: %d\n", header->device_rom_revision);
>> +
>> + switch (header->device_rom_revision) {
>> + case 3:
>> + res = usb4_drom_parse(sw);
>> + break;
>> + default:
>> + tb_sw_warn(sw, "DROM device_rom_revision %#x unknown\n",
>> + header->device_rom_revision);
>> + fallthrough;
>> + case 1:
>> + res = tb_drom_parse_v1(sw);
>> + break;
>> + }
>> +
>> + if (res) {
>> + tb_sw_warn(sw, "parsing DROM failed\n");
>> + goto err;
>> + }
>> +
>> + return 0;
> empty line
>
>> +err:
>> + kfree(sw->drom);
>> + sw->drom = NULL;
> empty line
>
>> + return -EIO;
>> +}
>> +
>> /**
>> * tb_drom_read() - Copy DROM to sw->drom and parse it
>> * @sw: Router whose DROM to read and parse
>> @@ -601,8 +668,7 @@ static int usb4_drom_parse(struct tb_switch *sw)
>> int tb_drom_read(struct tb_switch *sw)
>> {
>> u16 size;
>> - struct tb_drom_header *header;
>> - int res, retries = 1;
>> + int res;
>>
>> if (sw->drom)
>> return 0;
>> @@ -613,11 +679,11 @@ int tb_drom_read(struct tb_switch *sw)
>> * in a device property. Use it if available.
>> */
>> if (tb_drom_copy_efi(sw, &size) == 0)
>> - goto parse;
>> + return tb_drom_parse(sw, &size);
>>
>> /* Non-Apple hardware has the DROM as part of NVM */
>> if (tb_drom_copy_nvm(sw, &size) == 0)
>> - goto parse;
>> + return tb_drom_parse(sw, &size);
>>
>> /*
>> * USB4 hosts may support reading DROM through router
>> @@ -626,7 +692,7 @@ int tb_drom_read(struct tb_switch *sw)
>> if (tb_switch_is_usb4(sw)) {
>> usb4_switch_read_uid(sw, &sw->uid);
>> if (!usb4_copy_host_drom(sw, &size))
>> - goto parse;
>> + return tb_drom_parse(sw, &size);
>> } else {
>> /*
>> * The root switch contains only a dummy drom
>> @@ -640,67 +706,12 @@ int tb_drom_read(struct tb_switch *sw)
>> }
>>
>> /* We can use LC to get UUID later */
>> - if (sw->cap_lc && tb_drom_copy_nvm(sw, &size) == 0)
>> - goto parse;
>> -
>> - res = tb_drom_read_n(sw, 14, (u8 *) &size, 2);
>> + if (sw->cap_lc)
>> + res = tb_drom_copy_nvm(sw, &size);
>> + else
>> + res = tb_drom_bit_bang(sw, &size);
>> if (res)
>> return res;
>> - size &= 0x3ff;
>> - size += TB_DROM_DATA_START;
>> - tb_sw_dbg(sw, "reading drom (length: %#x)\n", size);
>> - if (size < sizeof(*header)) {
>> - tb_sw_warn(sw, "drom too small, aborting\n");
>> - return -EIO;
>> - }
>> -
>> - sw->drom = kzalloc(size, GFP_KERNEL);
>> - if (!sw->drom)
>> - return -ENOMEM;
>> -read:
>> - res = tb_drom_read_n(sw, 0, sw->drom, size);
>> - if (res)
>> - goto err;
>> -
>> -parse:
>> - header = (void *) sw->drom;
>> -
>> - if (header->data_len + TB_DROM_DATA_START != size) {
>> - tb_sw_warn(sw, "drom size mismatch\n");
>> - if (retries--) {
>> - msleep(100);
>> - goto read;
>> - }
>> - goto err;
>> - }
>> -
>> - tb_sw_dbg(sw, "DROM version: %d\n", header->device_rom_revision);
>> -
>> - switch (header->device_rom_revision) {
>> - case 3:
>> - res = usb4_drom_parse(sw);
>> - break;
>> - default:
>> - tb_sw_warn(sw, "DROM device_rom_revision %#x unknown\n",
>> - header->device_rom_revision);
>> - fallthrough;
>> - case 1:
>> - res = tb_drom_parse(sw);
>> - break;
>> - }
>> -
>> - /* If the DROM parsing fails, wait a moment and retry once */
>> - if (res == -EILSEQ && retries--) {
>> - tb_sw_warn(sw, "parsing DROM failed\n");
>> - msleep(100);
>> - goto read;
>> - }
>>
>> - if (!res)
>> - return 0;
>> -
>> -err:
>> - kfree(sw->drom);
>> - sw->drom = NULL;
>> - return -EIO;
>> + return tb_drom_parse(sw, &size);
>> }
>> --
>> 2.34.1
Powered by blists - more mailing lists