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