Re: [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921)

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