* [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921) @ 2022-09-21 15:51 Sachin Sant 2022-09-21 20:43 ` Kees Cook 0 siblings, 1 reply; 3+ messages in thread From: Sachin Sant @ 2022-09-21 15:51 UTC (permalink / raw) To: linuxppc-dev, linux-scsi; +Cc: linux-next, Kees Cook While booting recent linux-next kernel on a Power server following warning is seen: [ 6.427054] lpfc 0022:01:00.0: 0:6468 Set host date / time: Status x10: [ 6.471457] lpfc 0022:01:00.0: 0:6448 Dual Dump is enabled [ 7.432161] ------------[ cut here ]------------ [ 7.432178] memcpy: detected field-spanning write (size 8) of single field "&event->event_data" at drivers/scsi/scsi_transport_fc.c:581 (size 4) [ 7.432201] WARNING: CPU: 0 PID: 16 at drivers/scsi/scsi_transport_fc.c:581 fc_host_post_fc_event+0x214/0x300 [scsi_transport_fc] [ 7.432228] Modules linked in: sr_mod(E) cdrom(E) sd_mod(E) sg(E) lpfc(E+) nvmet_fc(E) ibmvscsi(E) nvmet(E) scsi_transport_srp(E) ibmveth(E) nvme_fc(E) nvme(E) nvme_fabrics(E) nvme_core(E) t10_pi(E) scsi_transport_fc(E) crc64_rocksoft(E) crc64(E) tg3(E) ipmi_devintf(E) ipmi_msghandler(E) fuse(E) [ 7.432263] CPU: 0 PID: 16 Comm: kworker/0:1 Tainted: G E 6.0.0-rc6-next-20220921 #38 [ 7.432270] Workqueue: events work_for_cpu_fn [ 7.432277] NIP: c008000001366a2c LR: c008000001366a28 CTR: 00000000007088ec [ 7.432282] REGS: c00000000380b6d0 TRAP: 0700 Tainted: G E (6.0.0-rc6-next-20220921) [ 7.432288] MSR: 800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 48002824 XER: 00000005 [ 7.432304] CFAR: c0000000001555b4 IRQMASK: 0 GPR00: c008000001366a28 c00000000380b970 c008000001388300 0000000000000084 GPR04: 00000000ffff7fff c00000000380b730 c00000000380b728 0000000000000027 GPR08: c000000db7007f98 0000000000000001 0000000000000027 c000000002947378 GPR12: 0000000000002000 c000000002dc0000 c00000000018e3d8 c000000003045740 GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR20: 0000000000000000 0000000000000030 01000000000010df c00000000380ba90 GPR24: 0000000000000001 c0000000030ea000 000000000000ffff c000000002da2a08 GPR28: 0000000000000040 c000000073f52400 0000000000000008 c0000000940b9834 [ 7.432365] NIP [c008000001366a2c] fc_host_post_fc_event+0x214/0x300 [scsi_transport_fc] [ 7.432374] LR [c008000001366a28] fc_host_post_fc_event+0x210/0x300 [scsi_transport_fc] [ 7.432383] Call Trace: [ 7.432385] [c00000000380b970] [c008000001366a28] fc_host_post_fc_event+0x210/0x300 [scsi_transport_fc] (unreliable) [ 7.432396] [c00000000380ba30] [c008000001c23028] lpfc_post_init_setup+0xc0/0x1f0 [lpfc] [ 7.432429] [c00000000380bab0] [c008000001c24e00] lpfc_pci_probe_one_s4.isra.59+0x428/0xa10 [lpfc] [ 7.432455] [c00000000380bb40] [c008000001c255a4] lpfc_pci_probe_one+0x1bc/0xb70 [lpfc] [ 7.432480] [c00000000380bbe0] [c0000000007fdc7c] local_pci_probe+0x6c/0x110 [ 7.432489] [c00000000380bc60] [c00000000017bdf8] work_for_cpu_fn+0x38/0x60 [ 7.432494] [c00000000380bc90] [c0000000001812d4] process_one_work+0x2b4/0x5b0 [ 7.432501] [c00000000380bd30] [c000000000181820] worker_thread+0x250/0x600 [ 7.432508] [c00000000380bdc0] [c00000000018e4f4] kthread+0x124/0x130 [ 7.432514] [c00000000380be10] [c00000000000cdf4] ret_from_kernel_thread+0x5c/0x64 [ 7.432521] Instruction dump: [ 7.432524] 2f890000 409eff5c 3ca20000 e8a58170 3c620000 e8638178 39200001 38c00004 [ 7.432535] 7fc4f378 992a0000 4800414d e8410018 <0fe00000> 7fa3eb78 38800001 480044d1 [ 7.432546] ---[ end trace 0000000000000000 ]--- [ 7.471075] lpfc 0022:01:00.0: 0:3176 Port Name 0 Physical Link is functional [ 7.471405] lpfc 0022:01:00.1: enabling device (0144 -> 0146) The warning was added by the following patch commit 54d9469bc515dc5fcbc20eecbe19cea868b70d68 fortify: Add run-time WARN for cross-field memcpy() Should this be fixed in the driver or is this a false warning? Thanks - Sachin ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921) 2022-09-21 15:51 [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921) Sachin Sant @ 2022-09-21 20:43 ` Kees Cook 2022-09-22 5:29 ` Sachin Sant 0 siblings, 1 reply; 3+ messages in thread From: Kees Cook @ 2022-09-21 20:43 UTC (permalink / raw) To: Sachin Sant; +Cc: linuxppc-dev, linux-scsi, linux-next On Wed, Sep 21, 2022 at 09:21:52PM +0530, Sachin Sant wrote: > While booting recent linux-next kernel on a Power server following > warning is seen: > > [ 6.427054] lpfc 0022:01:00.0: 0:6468 Set host date / time: Status x10: > [ 6.471457] lpfc 0022:01:00.0: 0:6448 Dual Dump is enabled > [ 7.432161] ------------[ cut here ]------------ > [ 7.432178] memcpy: detected field-spanning write (size 8) of single field "&event->event_data" at drivers/scsi/scsi_transport_fc.c:581 (size 4) Interesting! The memcpy() is this one: memcpy(&event->event_data, data_buf, data_len); The struct member, "event_data" is defined as u32: ... * Note: if Vendor Unique message, &event_data will be start of * Note: if Vendor Unique message, event_data_flex will be start of * vendor unique payload, and the length of the payload is * per event_datalen ... struct fc_nl_event { struct scsi_nl_hdr snlh; /* must be 1st element ! */ __u64 seconds; __u64 vendor_id; __u16 host_no; __u16 event_datalen; __u32 event_num; __u32 event_code; __u32 event_data; } __attribute__((aligned(sizeof(__u64)))); The warning says memcpy is trying to write 8 bytes into the 4 byte member, so the compiler is seeing it "correctly", but I think this is partially a false positive. It looks like there is also a small bug in the allocation size calculation and therefore a small leak of kernel heap memory contents. My notes: void fc_host_post_fc_event(struct Scsi_Host *shost, u32 event_number, enum fc_host_event_code event_code, u32 data_len, char *data_buf, u64 vendor_id) { ... struct fc_nl_event *event; ... if (!data_buf || data_len < 4) data_len = 0; This wants a data_buf and a data_len >= 4, so it does look like it's expected to be variable sized. There does appear to be an alignment and padding expectation, though: /* macro to round up message lengths to 8byte boundary */ #define FC_NL_MSGALIGN(len) (((len) + 7) & ~7) ... len = FC_NL_MSGALIGN(sizeof(*event) + data_len); But this is immediately suspicious: sizeof(*event) _includes_ event_data, so the alignment is going to be bumped up incorrectly. Note that struct fc_nl_event is 8 * 5 == 40 bytes, which allows for 4 bytes in event_data. But setting data_len to 4 (i.e. no "overflow") means we're asking for 44 bytes, which is aligned to 48. So, in all cases, there is uninitialized memory being sent... skb = nlmsg_new(len, GFP_KERNEL); ... nlh = nlmsg_put(skb, 0, 0, SCSI_TRANSPORT_MSG, len, 0); ... event = nlmsg_data(nlh); ... event->event_datalen = data_len; /* bytes */ Comments in the struct say this is counting from start of event_data. ... if (data_len) memcpy(&event->event_data, data_buf, data_len); And here is the reported memcpy(). The callers of fc_host_post_fc_event() are: fc_host_post_fc_event(shost, event_number, event_code, (u32)sizeof(u32), (char *)&event_data, 0); Fixed-size of 4 bytes: no "overflow". fc_host_post_fc_event(shost, event_number, FCH_EVT_VENDOR_UNIQUE, data_len, data_buf, vendor_id); fc_host_post_fc_event(shost, fc_get_event_number(), FCH_EVT_LINK_FPIN, fpin_len, fpin_buf, 0); These two appear to be of arbitrary length, but I didn't look more deeply. Given that the only user of struct fc_nl_event is fc_host_post_fc_event() in drivers/scsi/scsi_transport_fc.c, it looks safe to say that changing the struct to use a flexible array is the thing to do in the kernel, but we can't actually change the size or layout because it's a UAPI header. Are you able to test this patch: diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c index a2524106206d..0d798f11dc34 100644 --- a/drivers/scsi/scsi_transport_fc.c +++ b/drivers/scsi/scsi_transport_fc.c @@ -543,7 +543,7 @@ fc_host_post_fc_event(struct Scsi_Host *shost, u32 event_number, struct nlmsghdr *nlh; struct fc_nl_event *event; const char *name; - u32 len; + size_t len, padding; int err; if (!data_buf || data_len < 4) @@ -554,7 +554,7 @@ fc_host_post_fc_event(struct Scsi_Host *shost, u32 event_number, goto send_fail; } - len = FC_NL_MSGALIGN(sizeof(*event) + data_len); + len = FC_NL_MSGALIGN(sizeof(*event) - sizeof(event->event_data) + data_len); skb = nlmsg_new(len, GFP_KERNEL); if (!skb) { @@ -578,7 +578,9 @@ fc_host_post_fc_event(struct Scsi_Host *shost, u32 event_number, event->event_num = event_number; event->event_code = event_code; if (data_len) - memcpy(&event->event_data, data_buf, data_len); + memcpy(event->event_data_flex, data_buf, data_len); + padding = len - offsetof(typeof(*event), event_data_flex) - data_len; + memset(event->event_data_flex + data_len, 0, padding); nlmsg_multicast(scsi_nl_sock, skb, 0, SCSI_NL_GRP_FC_EVENTS, GFP_KERNEL); diff --git a/include/uapi/scsi/scsi_netlink_fc.h b/include/uapi/scsi/scsi_netlink_fc.h index 7535253f1a96..b46e9cbeb001 100644 --- a/include/uapi/scsi/scsi_netlink_fc.h +++ b/include/uapi/scsi/scsi_netlink_fc.h @@ -35,7 +35,7 @@ * FC Transport Broadcast Event Message : * FC_NL_ASYNC_EVENT * - * Note: if Vendor Unique message, &event_data will be start of + * Note: if Vendor Unique message, event_data_flex will be start of * vendor unique payload, and the length of the payload is * per event_datalen * @@ -50,7 +50,10 @@ struct fc_nl_event { __u16 event_datalen; __u32 event_num; __u32 event_code; - __u32 event_data; + union { + __u32 event_data; + __DECLARE_FLEX_ARRAY(__u8, event_data_flex); + }; } __attribute__((aligned(sizeof(__u64)))); -- Kees Cook ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921) 2022-09-21 20:43 ` Kees Cook @ 2022-09-22 5:29 ` Sachin Sant 0 siblings, 0 replies; 3+ messages in thread From: Sachin Sant @ 2022-09-22 5:29 UTC (permalink / raw) To: Kees Cook; +Cc: linuxppc-dev, linux-scsi, linux-next > On 22-Sep-2022, at 2:13 AM, Kees Cook <keescook@chromium.org> wrote: > > On Wed, Sep 21, 2022 at 09:21:52PM +0530, Sachin Sant wrote: >> While booting recent linux-next kernel on a Power server following >> warning is seen: >> >> [ 6.427054] lpfc 0022:01:00.0: 0:6468 Set host date / time: Status x10: >> [ 6.471457] lpfc 0022:01:00.0: 0:6448 Dual Dump is enabled >> [ 7.432161] ------------[ cut here ]------------ >> [ 7.432178] memcpy: detected field-spanning write (size 8) of single field "&event->event_data" at drivers/scsi/scsi_transport_fc.c:581 (size 4) > > Interesting! > > The memcpy() is this one: > > memcpy(&event->event_data, data_buf, data_len); > > The struct member, "event_data" is defined as u32: > > ... > * Note: if Vendor Unique message, &event_data will be start of > * Note: if Vendor Unique message, event_data_flex will be start of > * vendor unique payload, and the length of the payload is > * per event_datalen > ... > struct fc_nl_event { > struct scsi_nl_hdr snlh; /* must be 1st element ! */ > __u64 seconds; > __u64 vendor_id; > __u16 host_no; > __u16 event_datalen; > __u32 event_num; > __u32 event_code; > __u32 event_data; > } __attribute__((aligned(sizeof(__u64)))); > > The warning says memcpy is trying to write 8 bytes into the 4 byte > member, so the compiler is seeing it "correctly", but I think this is > partially a false positive. It looks like there is also a small bug in > the allocation size calculation and therefore a small leak of kernel > heap memory contents. My notes: > > void > fc_host_post_fc_event(struct Scsi_Host *shost, u32 event_number, > enum fc_host_event_code event_code, > u32 data_len, char *data_buf, u64 vendor_id) > { > ... > struct fc_nl_event *event; > ... > if (!data_buf || data_len < 4) > data_len = 0; > > This wants a data_buf and a data_len >= 4, so it does look like it's > expected to be variable sized. There does appear to be an alignment > and padding expectation, though: > > /* macro to round up message lengths to 8byte boundary */ > #define FC_NL_MSGALIGN(len) (((len) + 7) & ~7) > > ... > len = FC_NL_MSGALIGN(sizeof(*event) + data_len); > > But this is immediately suspicious: sizeof(*event) _includes_ event_data, > so the alignment is going to be bumped up incorrectly. Note that > struct fc_nl_event is 8 * 5 == 40 bytes, which allows for 4 bytes in > event_data. But setting data_len to 4 (i.e. no "overflow") means we're > asking for 44 bytes, which is aligned to 48. > > So, in all cases, there is uninitialized memory being sent... > > skb = nlmsg_new(len, GFP_KERNEL); > ... > nlh = nlmsg_put(skb, 0, 0, SCSI_TRANSPORT_MSG, len, 0); > ... > event = nlmsg_data(nlh); > ... > event->event_datalen = data_len; /* bytes */ > > Comments in the struct say this is counting from start of event_data. > > ... > if (data_len) > memcpy(&event->event_data, data_buf, data_len); > > And here is the reported memcpy(). > > The callers of fc_host_post_fc_event() are: > > fc_host_post_fc_event(shost, event_number, event_code, > (u32)sizeof(u32), (char *)&event_data, 0); > > Fixed-size of 4 bytes: no "overflow". > > fc_host_post_fc_event(shost, event_number, FCH_EVT_VENDOR_UNIQUE, > data_len, data_buf, vendor_id); > > fc_host_post_fc_event(shost, fc_get_event_number(), > FCH_EVT_LINK_FPIN, fpin_len, fpin_buf, 0); > > These two appear to be of arbitrary length, but I didn't look more > deeply. > > Given that the only user of struct fc_nl_event is fc_host_post_fc_event() > in drivers/scsi/scsi_transport_fc.c, it looks safe to say that changing > the struct to use a flexible array is the thing to do in the kernel, but > we can't actually change the size or layout because it's a UAPI header. > > Are you able to test this patch: Thank you for the detailed analysis. With this patch applied I don’t see the warning. Thanks - Sachin ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-22 5:29 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-21 15:51 [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921) Sachin Sant 2022-09-21 20:43 ` Kees Cook 2022-09-22 5:29 ` Sachin Sant
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).