linux-kernel - Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241213135321-mutt-send-email-mst@kernel.org>
Date: Fri, 13 Dec 2024 13:55:00 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Shijith Thotton <sthotton@...vell.com>
Cc: Jason Wang <jasowang@...hat.com>,
	"virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>,
	"dan.carpenter@...aro.org" <dan.carpenter@...aro.org>,
	Srujana Challa <schalla@...vell.com>,
	Vamsi Krishna Attunuru <vattunuru@...vell.com>,
	Nithin Kumar Dabilpuram <ndabilpuram@...vell.com>,
	Jerin Jacob <jerinj@...vell.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	Eugenio Pérez <eperezma@...hat.com>,
	Satha Koteswara Rao Kottidi <skoteshwar@...vell.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI
 capability

On Fri, Dec 13, 2024 at 02:20:24PM +0000, Shijith Thotton wrote:
> >>>
> >>> Added support to read the vendor-specific PCI capability to identify the
> >>> type of device being emulated.
> >>>
> >>> Reviewed-by: Dan Carpenter <dan.carpenter@...aro.org>
> >>> Signed-off-by: Shijith Thotton <sthotton@...vell.com>
> >>> ---
> >>>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
> >>>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33
> >>+++++++++++++++++++++++-
> >>>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
> >>>  3 files changed, 55 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>b/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> index 2cadb878e679..53b020b019f7 100644
> >>> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> @@ -8,6 +8,7 @@
> >>>  #include <linux/pci_regs.h>
> >>>  #include <linux/vdpa.h>
> >>>  #include <linux/virtio_pci_modern.h>
> >>> +#include <uapi/linux/virtio_crypto.h>
> >>>  #include <uapi/linux/virtio_net.h>
> >>>  #include <uapi/linux/virtio_blk.h>
> >>>  #include <uapi/linux/virtio_config.h>
> >>> @@ -52,6 +53,24 @@ struct octep_vring_info {
> >>>         phys_addr_t notify_pa;
> >>>  };
> >>>
> >>> +enum octep_pci_vndr_cfg_type {
> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
> >>> +};
> >>> +
> >>> +struct octep_pci_vndr_data {
> >>> +       struct virtio_pci_vndr_data hdr;
> >>> +       u8 id;
> >>> +       u8 bar;
> >>> +       union {
> >>> +               u64 data;
> >>> +               struct {
> >>> +                       u32 offset;
> >>> +                       u32 length;
> >>> +               };
> >>> +       };
> >>> +};
> >>
> >>This seems not to be padded to a multiple of 4 bytes?
> >>
> >
> >Looks like, the structure is naturally aligned.
> >
> >struct virtio_pci_vndr_data {
> >        uint8_t                    cap_vndr;             /*     0     1 */
> >        uint8_t                    cap_next;             /*     1     1 */
> >        uint8_t                    cap_len;              /*     2     1 */
> >        uint8_t                    cfg_type;             /*     3     1 */
> >        uint16_t                   vendor_id;            /*     4     2 */
> >        /* size: 6, cachelines: 1, members: 5 */
> >        /* last cacheline: 6 bytes */
> >};
> >struct octep_pci_vndr_data {
> >        struct virtio_pci_vndr_data hdr;                 /*     0     6 */
> >        uint8_t                    id;                   /*     6     1 */
> >        uint8_t                    bar;                  /*     7     1 */
> >        union {
> >                uint64_t           data;                 /*     8     8 */
> >                struct {
> >                        uint32_t   offset;               /*     8     4 */
> >                        uint32_t   length;               /*    12     4 */
> >                };                                       /*     8     8 */
> >        };                                               /*     8     8 */
> >
> >        /* size: 16, cachelines: 1, members: 4 */
> >        /* last cacheline: 16 bytes */
> >};
> >
> >I will add __attribute__((aligned(4))) to be safe.
> >
> 
> Small correction, I meant   __attribute__((packed, aligned(4))).
> 
> Thanks,
> Shijith

Don't add packed pls, for me it means "something is not packed naturally
here". when in fact it is.

aligned is also bad, since it can add padding which creates all
kind of security issues.

If you want to be sure, add BUILD_BUG_ON and check that it is aligned
as expected.