| 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: <CAGtn9rmJ2C=THWn351fH7s=PXTvOwag9P4_ecQx2_=cFCjs4Qg@mail.gmail.com>
Date: Tue, 21 May 2024 15:07:35 -0400
From: Ewan Milne <emilne@...hat.com>
To: Kees Cook <keescook@...omium.org>
Cc: "Martin K . Petersen" <martin.petersen@...cle.com>, Justin Stitt <justinstitt@...gle.com>,
Sathya Prakash <sathya.prakash@...adcom.com>,
Sreekanth Reddy <sreekanth.reddy@...adcom.com>,
Suganath Prabu Subramani <suganath-prabu.subramani@...adcom.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>, MPT-FusionLinux.pdl@...adcom.com,
linux-scsi@...r.kernel.org, Charles Bertsch <cbertsch@....net>,
Bart Van Assche <bvanassche@....org>, Andy Shevchenko <andy@...nel.org>,
Kashyap Desai <kashyap.desai@...adcom.com>, Sumit Saxena <sumit.saxena@...adcom.com>,
Nilesh Javali <njavali@...vell.com>, Andrew Morton <akpm@...ux-foundation.org>,
Himanshu Madhani <himanshu.madhani@...cle.com>, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, mpi3mr-linuxdrv.pdl@...adcom.com,
GR-QLogic-Storage-Upstream@...vell.com, Marco Patalano <mpatalan@...hat.com>
Subject: Re: [PATCH 3/5] scsi: mpt3sas: Avoid possible run-time warning with
long manufacturer strings
On Tue, Apr 9, 2024 at 10:32 PM Kees Cook <keescook@...omium.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@...omium.org>
> ---
> Cc: Justin Stitt <justinstitt@...gle.com>
> Cc: Sathya Prakash <sathya.prakash@...adcom.com>
> Cc: Sreekanth Reddy <sreekanth.reddy@...adcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@...adcom.com>
> Cc: "James E.J. Bottomley" <jejb@...ux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@...cle.com>
> Cc: MPT-FusionLinux.pdl@...adcom.com
> Cc: linux-scsi@...r.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@...hat.com>
Reviewed-by: Ewan D. Milne <emilne@...hat.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 ]---
Powered by blists - more mailing lists