From: Sachin Sant <sachinp@linux.ibm.com>
To: Kees Cook <keescook@chromium.org>
Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
linux-scsi@vger.kernel.org, linux-next@vger.kernel.org
Subject: Re: [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921)
Date: Thu, 22 Sep 2022 10:59:34 +0530 [thread overview]
Message-ID: <941B194C-3BB4-453B-822C-7280778AB3D0@linux.ibm.com> (raw)
In-Reply-To: <202209211250.3049C29@keescook>
> 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
prev parent reply other threads:[~2022-09-22 5:29 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=941B194C-3BB4-453B-822C-7280778AB3D0@linux.ibm.com \
--to=sachinp@linux.ibm.com \
--cc=keescook@chromium.org \
--cc=linux-next@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).