| Message ID | 20240410023155.2100422-3-keescook@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: Avoid possible run-time warning with long manufacturer strings | expand |
On Tue, Apr 9, 2024 at 10:32 PM Kees Cook <keescook@chromium.org> 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@chromium.org> > --- > Cc: Justin Stitt <justinstitt@google.com> > Cc: Sathya Prakash <sathya.prakash@broadcom.com> > Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: MPT-FusionLinux.pdl@broadcom.com > Cc: linux-scsi@vger.kernel.org > --- > 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@redhat.com> Reviewed-by: Ewan D. Milne <emilne@redhat.com 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 ]---
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 =
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@chromium.org> --- Cc: Justin Stitt <justinstitt@google.com> Cc: Sathya Prakash <sathya.prakash@broadcom.com> Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: MPT-FusionLinux.pdl@broadcom.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++++--------- 2 files changed, 6 insertions(+), 10 deletions(-)