* [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).