Re: [PATCH 3/5] scsi: mpt3sas: Avoid possible run-time warning with long manufacturer strings

[Ewan Milne]

On Tue, Apr 9, 2024 at 10:32 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> The prior strscpy() replacement of strncpy() here expected the
> manufacture_reply strings to be NUL-terminated, but it is possible
> they are not, as the code pattern here shows, e.g., edev->vendor_id
> being exactly 1 character larger than manufacture_reply->vendor_id,
> and the replaced strncpy() was copying only up to the size of the
> source character array. Replace this with memtostr(), which is the
> unambiguous way to convert a maybe not-NUL-terminated character array
> into a NUL-terminated string.
>
> Fixes: b7e9712a02e8 ("scsi: mpt3sas: Replace deprecated strncpy() with strscpy()")
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
> Cc: Sathya Prakash <sathya.prakash@xxxxxxxxxxxx>
> Cc: Sreekanth Reddy <sreekanth.reddy@xxxxxxxxxxxx>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@xxxxxxxxxxxx>
> Cc: "James E.J. Bottomley" <jejb@xxxxxxxxxxxxx>
> Cc: "Martin K. Petersen" <martin.petersen@xxxxxxxxxx>
> Cc: MPT-FusionLinux.pdl@xxxxxxxxxxxx
> Cc: linux-scsi@xxxxxxxxxxxxxxx
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c      |  2 +-
>  drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++++---------
>  2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 258647fc6bdd..1320e06727df 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -4774,7 +4774,7 @@ _base_display_ioc_capabilities(struct MPT3SAS_ADAPTER *ioc)
>         char desc[17] = {0};
>         u32 iounit_pg1_flags;
>
> -       strscpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
> +       memtostr(desc, ioc->manu_pg0.ChipName);
>         ioc_info(ioc, "%s: FWVersion(%02d.%02d.%02d.%02d), ChipRevision(0x%02x)\n",
>                  desc,
>                  (ioc->facts.FWVersion.Word & 0xFF000000) >> 24,
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> index 76f9a9177198..d84413b77d84 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> @@ -458,17 +458,13 @@ _transport_expander_report_manufacture(struct MPT3SAS_ADAPTER *ioc,
>                         goto out;
>
>                 manufacture_reply = data_out + sizeof(struct rep_manu_request);
> -               strscpy(edev->vendor_id, manufacture_reply->vendor_id,
> -                       sizeof(edev->vendor_id));
> -               strscpy(edev->product_id, manufacture_reply->product_id,
> -                       sizeof(edev->product_id));
> -               strscpy(edev->product_rev, manufacture_reply->product_rev,
> -                       sizeof(edev->product_rev));
> +               memtostr(edev->vendor_id, manufacture_reply->vendor_id);
> +               memtostr(edev->product_id, manufacture_reply->product_id);
> +               memtostr(edev->product_rev, manufacture_reply->product_rev);
>                 edev->level = manufacture_reply->sas_format & 1;
>                 if (edev->level) {
> -                       strscpy(edev->component_vendor_id,
> -                               manufacture_reply->component_vendor_id,
> -                               sizeof(edev->component_vendor_id));
> +                       memtostr(edev->component_vendor_id,
> +                                manufacture_reply->component_vendor_id);
>                         tmp = (u8 *)&manufacture_reply->component_id;
>                         edev->component_id = tmp[0] << 8 | tmp[1];
>                         edev->component_revision_id =
> --
> 2.34.1
>
>

Tested-by: Marco Patalano <mpatalan@xxxxxxxxxx>
Reviewed-by: Ewan D. Milne <emilne@xxxxxxxxxx

This fixes the following warning & subsequent panic seen on one of our
test machines:

[    4.986905] ------------[ cut here ]------------
[    4.991545] strnlen: detected buffer overflow: 9 byte read of buffer size 8
[    4.998528] WARNING: CPU: 2 PID: 13 at lib/string_helpers.c:1029
__fortify_report+0x3f/0x50
[    5.006889] Modules linked in: qla2xxx(+) bnxt_en mpt3sas(+)
nvme_fc ahci crct10dif_pclmul libahci nvme_fabrics crc32_pclmul
crc32c_intel nvme_core libata t10_pi raid_class ghash_clmulni_intel
tg3 scsi_transport_fc scsi_transport_sas dimlib wmi dm_mirror
dm_region_hash dm_log dm_mod
[    5.031912] CPU: 2 PID: 13 Comm: kworker/u128:1 Not tainted 6.9.0+ #1
[    5.038352] Hardware name: Dell Inc. PowerEdge R640/06NR82, BIOS
2.21.2 02/19/2024
[    5.038355] Workqueue: fw_event_mpt3sas0 _firmware_event_work [mpt3sas]
[    5.052557] RIP: 0010:__fortify_report+0x3f/0x50
[    5.052560] Code: c1 83 e7 01 48 c7 c1 5c 4d 08 b9 48 c7 c7 80 c1
01 b9 48 8b 34 c5 80 cc af b8 48 c7 c0 66 4d 08 b9 48 0f 45 c8 e8 01
a8 a8 ff <0f> 0b c3 cc cc cc cc 66 2e 0f 1f 84 00 00 00 00 00 90 90 90
90 90
[    5.075926] RSP: 0018:ffffb972c02c7bb0 EFLAGS: 00010286
[    5.075928] RAX: 0000000000000000 RBX: ffff915503b12100 RCX: 0000000000000000
[    5.088284] RDX: ffff91586faaee40 RSI: ffff91586faa0bc0 RDI: ffff91586faa0bc0
[    5.095418] RBP: ffff915503f11c08 R08: 0000000000000000 R09: ffffb972c02c7a60
[    5.102549] R10: ffffb972c02c7a58 R11: ffffffffb95deba8 R12: ffff9155126ef010
[    5.102551] R13: ffff9155086ecb50 R14: ffff9155126ef000 R15: ffff9155086ec848
[    5.102552] FS:  0000000000000000(0000) GS:ffff91586fa80000(0000)
knlGS:0000000000000000
[    5.116816] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    5.120583] ata15.00: Security Log not supported
[    5.120723] ata15.00: Security Log not supported
[    5.130645] CR2: 00007f48a3d47a50 CR3: 00000003b2e20006 CR4: 00000000007706f0
[    5.130647] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    5.139880] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    5.139882] PKRU: 55555554
[    5.139883] Call Trace:
[    5.154146]  <TASK>
[    5.163991]  ? __warn+0x7f/0x120
[    5.168549]  ? __fortify_report+0x3f/0x50
[    5.175791]  ? report_bug+0x18a/0x1a0
[    5.179479]  ? handle_bug+0x3c/0x70
[    5.182994]  ? exc_invalid_op+0x14/0x70
[    5.182997]  ? asm_exc_invalid_op+0x16/0x20
[    5.191021]  ? __fortify_report+0x3f/0x50
[    5.195033]  __fortify_panic+0x9/0x10
[    5.198699]
_transport_expander_report_manufacture.isra.0+0x5f0/0x620 [mpt3sas]
[    5.206145]  mpt3sas_transport_port_add+0x5df/0x7a0 [mpt3sas]
[    5.211931]  _scsih_expander_add+0x28a/0x650 [mpt3sas]
[    5.217112]  ? _scsih_sas_host_refresh+0x2aa/0x510 [mpt3sas]
[    5.222799]  _scsih_sas_topology_change_event.isra.0+0x213/0x440 [mpt3sas]
[    5.229714]  _mpt3sas_fw_work+0x6ab/0xb50 [mpt3sas]
[    5.234636]  ? pick_next_task+0x9e2/0xae0
[    5.238649]  ? finish_task_switch.isra.0+0x97/0x290
[    5.243555]  ? move_linked_works+0x70/0xa0
[    5.247661]  process_one_work+0x184/0x3b0
[    5.251673]  worker_thread+0x2f9/0x410
[    5.251677]  ? __pfx_worker_thread+0x10/0x10
[    5.251679]  kthread+0xcc/0x100
[    5.259713]  ? __pfx_kthread+0x10/0x10
[    5.259715]  ret_from_fork+0x2d/0x50
[    5.270190]  ? __pfx_kthread+0x10/0x10
[    5.273944]  ret_from_fork_asm+0x1a/0x30
[    5.277872]  </TASK>
[    5.280062] ---[ end trace 0000000000000000 ]---