| Message ID | 20220921205155.1451649-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: scsi_transport_fc: Adjust struct fc_nl_event flex array usage | expand |
> On 22-Sep-2022, at 2:21 AM, Kees Cook <keescook@chromium.org> wrote: > > In order to help the compiler reason about the destination buffer in > struct fc_nl_event, add a flexible array member for this purpose. > However, since the header is UAPI, it must not change size or layout, so > a union is used. > > The allocation size calculations are also corrected (it was potentially > allocating an extra 8 bytes), and the padding is zeroed to avoid leaking > kernel heap memory contents. > > Detected at run-time by the recently added memcpy() bounds checking: > > memcpy: detected field-spanning write (size 8) of single field "&event->event_data" at drivers/scsi/scsi_transport_fc.c:581 (size 4) > > Reported-by: Sachin Sant <sachinp@linux.ibm.com> > Link: https://lore.kernel.org/linux-next/42404B5E-198B-4FD3-94D6-5E16CF579EF3@linux.ibm.com/ > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- Thanks for the fix. With this patch applied the warning is no longer seen. Tested-by: Sachin Sant <sachinp@linux.ibm.com> > drivers/scsi/scsi_transport_fc.c | 8 +++++--- > include/uapi/scsi/scsi_netlink_fc.h | 7 +++++-- > 2 files changed, 10 insertions(+), 5 deletions(-) > > 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)))); > > > -- > 2.34.1 >
On 9/21/2022 1:51 PM, Kees Cook wrote: > In order to help the compiler reason about the destination buffer in > struct fc_nl_event, add a flexible array member for this purpose. > However, since the header is UAPI, it must not change size or layout, so > a union is used. > > The allocation size calculations are also corrected (it was potentially > allocating an extra 8 bytes), and the padding is zeroed to avoid leaking > kernel heap memory contents. > > Detected at run-time by the recently added memcpy() bounds checking: > > memcpy: detected field-spanning write (size 8) of single field "&event->event_data" at drivers/scsi/scsi_transport_fc.c:581 (size 4) > > Reported-by: Sachin Sant <sachinp@linux.ibm.com> > Link: https://lore.kernel.org/linux-next/42404B5E-198B-4FD3-94D6-5E16CF579EF3@linux.ibm.com/ > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- Kinda crazy way to resolve it, but looks fine. Reviewed-by: James Smart <jsmart2021@gmail.com> -- james
Kees, > In order to help the compiler reason about the destination buffer in > struct fc_nl_event, add a flexible array member for this purpose. > However, since the header is UAPI, it must not change size or layout, > so a union is used. Applied to 6.1/scsi-staging, thanks!
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))));
In order to help the compiler reason about the destination buffer in struct fc_nl_event, add a flexible array member for this purpose. However, since the header is UAPI, it must not change size or layout, so a union is used. The allocation size calculations are also corrected (it was potentially allocating an extra 8 bytes), and the padding is zeroed to avoid leaking kernel heap memory contents. Detected at run-time by the recently added memcpy() bounds checking: memcpy: detected field-spanning write (size 8) of single field "&event->event_data" at drivers/scsi/scsi_transport_fc.c:581 (size 4) Reported-by: Sachin Sant <sachinp@linux.ibm.com> Link: https://lore.kernel.org/linux-next/42404B5E-198B-4FD3-94D6-5E16CF579EF3@linux.ibm.com/ Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: linux-scsi@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/scsi/scsi_transport_fc.c | 8 +++++--- include/uapi/scsi/scsi_netlink_fc.h | 7 +++++-- 2 files changed, 10 insertions(+), 5 deletions(-)