| Message ID | 20210707081625.5066-1-shravankr@nvidia.com |
|---|---|
| Headers | show |
| Series | Updates to mlx-bootctl | expand |
Acked-by: Tim Gardner <tim.gardner@canonical.com> There is a 'for' loop in rsh_log_show_crash() that _might_ overflow 'buf'. It depends on how the kernel implementation of snprintf() handles a length of 0. A way around that is: for (i = 0; i < len/2 && size > 0; i++) { This patch series is fine as is. If you're concerned about that 'for' loop, then you can send a follow-on patch that references the same bug number. rtg On 7/7/21 2:16 AM, Shravan Kumar Ramani wrote: > v4 --> v5 > Add more details to commit message and include missing "BugLink:" key. > Use one common bug report for all patches in the series. > > v3 --> v4 > Redo split as follows: > 1. Fix exclusion issues around arm_smccc_smc > 2. Fix potential buffer overflow > 3. Support VPD info in EEPROM MFG > 4. Update license and version info > > v2 --> v3 > Add mutex lock/unlock for SMC calls in show functions, similar to store. > Use PAGE_SIZE macro as buffer size for snprintf calls in DRIVER_ATTR_RW functions. > Use snprintf instead of sprintf in post_reset_wdog_show. > In secure_boot_fuse_state_show, the string comes from the function itself unlike the rest. So it is protected against buffer overflow. > > v1 --> v2 > Split single patch into 3 separate patches based on functionality as suggested. > > BugLink: https://bugs.launchpad.net/bugs/1931843 > SRU Justification: > > [Impact] > The EEPROM MFG partition on BlueField-2 has been updated to include the VPD information for each card. In order to access these newly added fields, the mlx-bootctl driver needs to be updated to provide an access mechanism. > > [Fix] > Add support for VPD fields in the EEPROM MFG and provide access to these via sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str sysfs has been renamed to opn. > > [Test Case] > Though the driver provides read and write access through sysfs, the contents of the MFG partition are written during Manufacturing and then locked in order to protect the info. Writing to this partition will therefore require resetting the MFG info from the UEFI Device Manager, which will unlock the partition and allow for it to be reprogrammed. > Reading the sysfs entries will print the contents of each field. It could also be empty if the field was not programmed earlier. > > [Regression Potential] > Can be considered minimum, since the new fields have been added without interfering with the existing fields which might already be present in the EEPROM. > > This patch series contains the following 4 patches: > 1. Fix exclusion issues around arm_smccc_smc > Exclusion is being handled around arm_smccc_smc() only in the store functions. It should be implemented in the show functions also as this call might not be thread-safe. > Add mutex_lock/unlock around arm_smccc_smc calls in the show functions. > > 2. Fix potential buffer overflow > The sysfs store/show functions use sprintf without specifying a size which could lead to potential buffer overflow. > Replace sprintf with snprintf to avoid buffer overflow. Also, remove the redundant strlen usage since count is already available in the _store functions. > > 3. Support VPD info in EEPROM MFG > The EEPROM MFG partition on BlueField-2 has been updated to include the VPD information for each card. In order to access these newly added fields, the mlx-bootctl driver needs to be updated to provide an access mechanism. > Add support for VPD fields in the EEPROM MFG and provide access to these via sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str sysfs has been renamed to opn. > > 4. Update license and version info > Update license info to "Dual BSD/GPL". > Increment version to 1.4 > > Shravan Kumar Ramani (4): > UBUNTU: SAUCE: mlx-bootctl: Fix exclusion issues around arm_smccc_smc > UBUNTU: SAUCE: mlx-bootctl: Fix potential buffer overflow > UBUNTU: SAUCE: mlx-bootctl: Support VPD info in EEPROM MFG > UBUNTU: SAUCE: mlx-bootctl: Update license and version info > > drivers/platform/mellanox/mlx-bootctl.c | 373 ++++++++++++++++++------ > 1 file changed, 290 insertions(+), 83 deletions(-) >
On 07.07.21 14:53, Tim Gardner wrote: > Acked-by: Tim Gardner <tim.gardner@canonical.com> > > There is a 'for' loop in rsh_log_show_crash() that _might_ overflow 'buf'. It > depends on how the kernel implementation of snprintf() handles a length of 0. A > way around that is: > > for (i = 0; i < len/2 && size > 0; i++) { > > This patch series is fine as is. If you're concerned about that 'for' loop, then > you can send a follow-on patch that references the same bug number. Not if this set has already been applied by then. Partial adjustments under the same bug reference have a high risk of going wrong and once committed it is also causing confusion. Either the whole set should be NACKed and and re-submitted or the loop issue should be handled in a separate thread. -Stefan > > rtg > > On 7/7/21 2:16 AM, Shravan Kumar Ramani wrote: >> v4 --> v5 >> Add more details to commit message and include missing "BugLink:" key. >> Use one common bug report for all patches in the series. >> >> v3 --> v4 >> Redo split as follows: >> 1. Fix exclusion issues around arm_smccc_smc >> 2. Fix potential buffer overflow >> 3. Support VPD info in EEPROM MFG >> 4. Update license and version info >> >> v2 --> v3 >> Add mutex lock/unlock for SMC calls in show functions, similar to store. >> Use PAGE_SIZE macro as buffer size for snprintf calls in DRIVER_ATTR_RW >> functions. >> Use snprintf instead of sprintf in post_reset_wdog_show. >> In secure_boot_fuse_state_show, the string comes from the function itself >> unlike the rest. So it is protected against buffer overflow. >> >> v1 --> v2 >> Split single patch into 3 separate patches based on functionality as suggested. >> >> BugLink: https://bugs.launchpad.net/bugs/1931843 >> SRU Justification: >> >> [Impact] >> The EEPROM MFG partition on BlueField-2 has been updated to include the VPD >> information for each card. In order to access these newly added fields, the >> mlx-bootctl driver needs to be updated to provide an access mechanism. >> >> [Fix] >> Add support for VPD fields in the EEPROM MFG and provide access to these via >> sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model >> Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str >> sysfs has been renamed to opn. >> >> [Test Case] >> Though the driver provides read and write access through sysfs, the contents >> of the MFG partition are written during Manufacturing and then locked in order >> to protect the info. Writing to this partition will therefore require >> resetting the MFG info from the UEFI Device Manager, which will unlock the >> partition and allow for it to be reprogrammed. >> Reading the sysfs entries will print the contents of each field. It could also >> be empty if the field was not programmed earlier. >> >> [Regression Potential] >> Can be considered minimum, since the new fields have been added without >> interfering with the existing fields which might already be present in the >> EEPROM. >> >> This patch series contains the following 4 patches: >> 1. Fix exclusion issues around arm_smccc_smc >> Exclusion is being handled around arm_smccc_smc() only in the store functions. >> It should be implemented in the show functions also as this call might not be >> thread-safe. >> Add mutex_lock/unlock around arm_smccc_smc calls in the show functions. >> >> 2. Fix potential buffer overflow >> The sysfs store/show functions use sprintf without specifying a size which >> could lead to potential buffer overflow. >> Replace sprintf with snprintf to avoid buffer overflow. Also, remove the >> redundant strlen usage since count is already available in the _store functions. >> >> 3. Support VPD info in EEPROM MFG >> The EEPROM MFG partition on BlueField-2 has been updated to include the VPD >> information for each card. In order to access these newly added fields, the >> mlx-bootctl driver needs to be updated to provide an access mechanism. >> Add support for VPD fields in the EEPROM MFG and provide access to these via >> sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model >> Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str >> sysfs has been renamed to opn. >> >> 4. Update license and version info >> Update license info to "Dual BSD/GPL". >> Increment version to 1.4 >> >> Shravan Kumar Ramani (4): >> UBUNTU: SAUCE: mlx-bootctl: Fix exclusion issues around arm_smccc_smc >> UBUNTU: SAUCE: mlx-bootctl: Fix potential buffer overflow >> UBUNTU: SAUCE: mlx-bootctl: Support VPD info in EEPROM MFG >> UBUNTU: SAUCE: mlx-bootctl: Update license and version info >> >> drivers/platform/mellanox/mlx-bootctl.c | 373 ++++++++++++++++++------ >> 1 file changed, 290 insertions(+), 83 deletions(-) >> >
Noted, I will fix the loop issue in a separate thread. Thanks, Shravan > -----Original Message----- > From: Stefan Bader <stefan.bader@canonical.com> > Sent: Thursday, July 8, 2021 12:32 PM > To: Tim Gardner <tim.gardner@canonical.com>; Shravan Ramani > <shravankr@nvidia.com>; kernel-team@lists.ubuntu.com > Subject: Re: ACK/Cmnt: [SRU][F:linux-bluefield][PATCH v5 0/4] Updates to > mlx-bootctl > > On 07.07.21 14:53, Tim Gardner wrote: > > Acked-by: Tim Gardner <tim.gardner@canonical.com> > > > > There is a 'for' loop in rsh_log_show_crash() that _might_ overflow > > 'buf'. It depends on how the kernel implementation of snprintf() > > handles a length of 0. A way around that is: > > > > for (i = 0; i < len/2 && size > 0; i++) { > > > > This patch series is fine as is. If you're concerned about that 'for' > > loop, then you can send a follow-on patch that references the same bug > number. > > Not if this set has already been applied by then. Partial adjustments under > the same bug reference have a high risk of going wrong and once committed > it is also causing confusion. Either the whole set should be NACKed and and > re-submitted or the loop issue should be handled in a separate thread. > > -Stefan > > > > > rtg > > > > On 7/7/21 2:16 AM, Shravan Kumar Ramani wrote: > >> v4 --> v5 > >> Add more details to commit message and include missing "BugLink:" key. > >> Use one common bug report for all patches in the series. > >> > >> v3 --> v4 > >> Redo split as follows: > >> 1. Fix exclusion issues around arm_smccc_smc 2. Fix potential buffer > >> overflow 3. Support VPD info in EEPROM MFG 4. Update license and > >> version info > >> > >> v2 --> v3 > >> Add mutex lock/unlock for SMC calls in show functions, similar to store. > >> Use PAGE_SIZE macro as buffer size for snprintf calls in > >> DRIVER_ATTR_RW functions. > >> Use snprintf instead of sprintf in post_reset_wdog_show. > >> In secure_boot_fuse_state_show, the string comes from the function > >> itself unlike the rest. So it is protected against buffer overflow. > >> > >> v1 --> v2 > >> Split single patch into 3 separate patches based on functionality as > suggested. > >> > >> BugLink: https://bugs.launchpad.net/bugs/1931843 > >> SRU Justification: > >> > >> [Impact] > >> The EEPROM MFG partition on BlueField-2 has been updated to include > >> the VPD information for each card. In order to access these newly > >> added fields, the mlx-bootctl driver needs to be updated to provide an > access mechanism. > >> > >> [Fix] > >> Add support for VPD fields in the EEPROM MFG and provide access to > >> these via sysfs entries. The newly added sysfs entries are: sku (SKU > >> ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the > >> previously added opn_str sysfs has been renamed to opn. > >> > >> [Test Case] > >> Though the driver provides read and write access through sysfs, the > >> contents of the MFG partition are written during Manufacturing and > >> then locked in order to protect the info. Writing to this partition > >> will therefore require resetting the MFG info from the UEFI Device > >> Manager, which will unlock the partition and allow for it to be > reprogrammed. > >> Reading the sysfs entries will print the contents of each field. It > >> could also be empty if the field was not programmed earlier. > >> > >> [Regression Potential] > >> Can be considered minimum, since the new fields have been added > >> without interfering with the existing fields which might already be > >> present in the EEPROM. > >> > >> This patch series contains the following 4 patches: > >> 1. Fix exclusion issues around arm_smccc_smc Exclusion is being > >> handled around arm_smccc_smc() only in the store functions. > >> It should be implemented in the show functions also as this call > >> might not be thread-safe. > >> Add mutex_lock/unlock around arm_smccc_smc calls in the show > functions. > >> > >> 2. Fix potential buffer overflow > >> The sysfs store/show functions use sprintf without specifying a size > >> which could lead to potential buffer overflow. > >> Replace sprintf with snprintf to avoid buffer overflow. Also, remove > >> the redundant strlen usage since count is already available in the _store > functions. > >> > >> 3. Support VPD info in EEPROM MFG > >> The EEPROM MFG partition on BlueField-2 has been updated to include > >> the VPD information for each card. In order to access these newly > >> added fields, the mlx-bootctl driver needs to be updated to provide an > access mechanism. > >> Add support for VPD fields in the EEPROM MFG and provide access to > >> these via sysfs entries. The newly added sysfs entries are: sku (SKU > >> ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the > >> previously added opn_str sysfs has been renamed to opn. > >> > >> 4. Update license and version info > >> Update license info to "Dual BSD/GPL". > >> Increment version to 1.4 > >> > >> Shravan Kumar Ramani (4): > >> UBUNTU: SAUCE: mlx-bootctl: Fix exclusion issues around > >> arm_smccc_smc > >> UBUNTU: SAUCE: mlx-bootctl: Fix potential buffer overflow > >> UBUNTU: SAUCE: mlx-bootctl: Support VPD info in EEPROM MFG > >> UBUNTU: SAUCE: mlx-bootctl: Update license and version info > >> > >> drivers/platform/mellanox/mlx-bootctl.c | 373 > >> ++++++++++++++++++------ > >> 1 file changed, 290 insertions(+), 83 deletions(-) > >> > > >
On 07.07.21 10:16, Shravan Kumar Ramani wrote: > v4 --> v5 > Add more details to commit message and include missing "BugLink:" key. > Use one common bug report for all patches in the series. > > v3 --> v4 > Redo split as follows: > 1. Fix exclusion issues around arm_smccc_smc > 2. Fix potential buffer overflow > 3. Support VPD info in EEPROM MFG > 4. Update license and version info > > v2 --> v3 > Add mutex lock/unlock for SMC calls in show functions, similar to store. > Use PAGE_SIZE macro as buffer size for snprintf calls in DRIVER_ATTR_RW functions. > Use snprintf instead of sprintf in post_reset_wdog_show. > In secure_boot_fuse_state_show, the string comes from the function itself unlike the rest. So it is protected against buffer overflow. > > v1 --> v2 > Split single patch into 3 separate patches based on functionality as suggested. > > BugLink: https://bugs.launchpad.net/bugs/1931843 > SRU Justification: > > [Impact] > The EEPROM MFG partition on BlueField-2 has been updated to include the VPD information for each card. In order to access these newly added fields, the mlx-bootctl driver needs to be updated to provide an access mechanism. > > [Fix] > Add support for VPD fields in the EEPROM MFG and provide access to these via sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str sysfs has been renamed to opn. > > [Test Case] > Though the driver provides read and write access through sysfs, the contents of the MFG partition are written during Manufacturing and then locked in order to protect the info. Writing to this partition will therefore require resetting the MFG info from the UEFI Device Manager, which will unlock the partition and allow for it to be reprogrammed. > Reading the sysfs entries will print the contents of each field. It could also be empty if the field was not programmed earlier. > > [Regression Potential] > Can be considered minimum, since the new fields have been added without interfering with the existing fields which might already be present in the EEPROM. > > This patch series contains the following 4 patches: > 1. Fix exclusion issues around arm_smccc_smc > Exclusion is being handled around arm_smccc_smc() only in the store functions. It should be implemented in the show functions also as this call might not be thread-safe. > Add mutex_lock/unlock around arm_smccc_smc calls in the show functions. > > 2. Fix potential buffer overflow > The sysfs store/show functions use sprintf without specifying a size which could lead to potential buffer overflow. > Replace sprintf with snprintf to avoid buffer overflow. Also, remove the redundant strlen usage since count is already available in the _store functions. > > 3. Support VPD info in EEPROM MFG > The EEPROM MFG partition on BlueField-2 has been updated to include the VPD information for each card. In order to access these newly added fields, the mlx-bootctl driver needs to be updated to provide an access mechanism. > Add support for VPD fields in the EEPROM MFG and provide access to these via sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str sysfs has been renamed to opn. > > 4. Update license and version info > Update license info to "Dual BSD/GPL". > Increment version to 1.4 > > Shravan Kumar Ramani (4): > UBUNTU: SAUCE: mlx-bootctl: Fix exclusion issues around arm_smccc_smc > UBUNTU: SAUCE: mlx-bootctl: Fix potential buffer overflow > UBUNTU: SAUCE: mlx-bootctl: Support VPD info in EEPROM MFG > UBUNTU: SAUCE: mlx-bootctl: Update license and version info > > drivers/platform/mellanox/mlx-bootctl.c | 373 ++++++++++++++++++------ > 1 file changed, 290 insertions(+), 83 deletions(-) > Acked-by: Stefan Bader <stefan.bader@canonical.com>
On 07.07.21 10:16, Shravan Kumar Ramani wrote: > v4 --> v5 > Add more details to commit message and include missing "BugLink:" key. > Use one common bug report for all patches in the series. > > v3 --> v4 > Redo split as follows: > 1. Fix exclusion issues around arm_smccc_smc > 2. Fix potential buffer overflow > 3. Support VPD info in EEPROM MFG > 4. Update license and version info > > v2 --> v3 > Add mutex lock/unlock for SMC calls in show functions, similar to store. > Use PAGE_SIZE macro as buffer size for snprintf calls in DRIVER_ATTR_RW functions. > Use snprintf instead of sprintf in post_reset_wdog_show. > In secure_boot_fuse_state_show, the string comes from the function itself unlike the rest. So it is protected against buffer overflow. > > v1 --> v2 > Split single patch into 3 separate patches based on functionality as suggested. > > BugLink: https://bugs.launchpad.net/bugs/1931843 > SRU Justification: > > [Impact] > The EEPROM MFG partition on BlueField-2 has been updated to include the VPD information for each card. In order to access these newly added fields, the mlx-bootctl driver needs to be updated to provide an access mechanism. > > [Fix] > Add support for VPD fields in the EEPROM MFG and provide access to these via sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str sysfs has been renamed to opn. > > [Test Case] > Though the driver provides read and write access through sysfs, the contents of the MFG partition are written during Manufacturing and then locked in order to protect the info. Writing to this partition will therefore require resetting the MFG info from the UEFI Device Manager, which will unlock the partition and allow for it to be reprogrammed. > Reading the sysfs entries will print the contents of each field. It could also be empty if the field was not programmed earlier. > > [Regression Potential] > Can be considered minimum, since the new fields have been added without interfering with the existing fields which might already be present in the EEPROM. > > This patch series contains the following 4 patches: > 1. Fix exclusion issues around arm_smccc_smc > Exclusion is being handled around arm_smccc_smc() only in the store functions. It should be implemented in the show functions also as this call might not be thread-safe. > Add mutex_lock/unlock around arm_smccc_smc calls in the show functions. > > 2. Fix potential buffer overflow > The sysfs store/show functions use sprintf without specifying a size which could lead to potential buffer overflow. > Replace sprintf with snprintf to avoid buffer overflow. Also, remove the redundant strlen usage since count is already available in the _store functions. > > 3. Support VPD info in EEPROM MFG > The EEPROM MFG partition on BlueField-2 has been updated to include the VPD information for each card. In order to access these newly added fields, the mlx-bootctl driver needs to be updated to provide an access mechanism. > Add support for VPD fields in the EEPROM MFG and provide access to these via sysfs entries. The newly added sysfs entries are: sku (SKU ID), modl (Model Number), sn (Serial Number) and uuid (UUID). And the previously added opn_str sysfs has been renamed to opn. > > 4. Update license and version info > Update license info to "Dual BSD/GPL". > Increment version to 1.4 > > Shravan Kumar Ramani (4): > UBUNTU: SAUCE: mlx-bootctl: Fix exclusion issues around arm_smccc_smc > UBUNTU: SAUCE: mlx-bootctl: Fix potential buffer overflow > UBUNTU: SAUCE: mlx-bootctl: Support VPD info in EEPROM MFG > UBUNTU: SAUCE: mlx-bootctl: Update license and version info > > drivers/platform/mellanox/mlx-bootctl.c | 373 ++++++++++++++++++------ > 1 file changed, 290 insertions(+), 83 deletions(-) > Applied to focal:linux-bluefield/master-next. Thanks. -Stefan