| 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
| ||
|
Date: Tue, 25 Oct 2022 13:32:59 -0700
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Igor Skalkin <igor.skalkin@...nsynergy.com>
Cc: virtualization@...ts.linux-foundation.org, mst@...hat.com,
marcel@...tmann.org, johan.hedberg@...il.com, jasowang@...hat.com,
linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/1] virtio_bt: Fix alignment in configuration struct
Hi Igor,
On Tue, Oct 25, 2022 at 1:17 PM Igor Skalkin
<igor.skalkin@...nsynergy.com> wrote:
>
> Hi Luiz Augusto,
>
> On 10/24/22 22:54, Luiz Augusto von Dentz wrote:
> > Hi Igor,
> >
> > On Mon, Oct 24, 2022 at 6:41 AM Igor Skalkin
> > <Igor.Skalkin@...nsynergy.com> wrote:
> >>
> >> The current version of the configuration structure has unaligned
> >> 16-bit fields, but according to the specification [1], access to
> >> the configuration space must be aligned.
> >>
> >> Add a second, aligned version of the configuration structure
> >> and a new feature bit indicating that this version is being used.
> >>
> >> [1] https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.oasis%2dopen.org%2fvirtio%2fvirtio%2fv1.1%2fvirtio%2dv1.1.pdf&umid=db3482bc-5b84-4bde-bbb0-41d837955a7a&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-d27a9d4c2c971f9ecc5d00d40d5cd9b45c4b5f63
> >>
> >> Signed-off-by: Igor Skalkin <Igor.Skalkin@...nsynergy.com>
> >> ---
> >> drivers/bluetooth/virtio_bt.c | 16 +++++++++++++---
> >> include/uapi/linux/virtio_bt.h | 8 ++++++++
> >> 2 files changed, 21 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
> >> index 67c21263f9e0..35f8041722c8 100644
> >> --- a/drivers/bluetooth/virtio_bt.c
> >> +++ b/drivers/bluetooth/virtio_bt.c
> >> @@ -306,7 +306,12 @@ static int virtbt_probe(struct virtio_device *vdev)
> >> if (virtio_has_feature(vdev, VIRTIO_BT_F_VND_HCI)) {
> >> __u16 vendor;
> >>
> >> - virtio_cread(vdev, struct virtio_bt_config, vendor, &vendor);
> >> + if (virtio_has_feature(vdev, VIRTIO_BT_F_CONFIG_V2))
> >> + virtio_cread(vdev, struct virtio_bt_config_v2,
> >> + vendor, &vendor);
> >> + else
> >> + virtio_cread(vdev, struct virtio_bt_config,
> >> + vendor, &vendor);
> >>
> >> switch (vendor) {
> >> case VIRTIO_BT_CONFIG_VENDOR_ZEPHYR:
> >> @@ -339,8 +344,12 @@ static int virtbt_probe(struct virtio_device *vdev)
> >> if (virtio_has_feature(vdev, VIRTIO_BT_F_MSFT_EXT)) {
> >> __u16 msft_opcode;
> >>
> >> - virtio_cread(vdev, struct virtio_bt_config,
> >> - msft_opcode, &msft_opcode);
> >> + if (virtio_has_feature(vdev, VIRTIO_BT_F_CONFIG_V2))
> >> + virtio_cread(vdev, struct virtio_bt_config_v2,
> >> + msft_opcode, &msft_opcode);
> >> + else
> >> + virtio_cread(vdev, struct virtio_bt_config,
> >> + msft_opcode, &msft_opcode);
> >>
> >> hci_set_msft_opcode(hdev, msft_opcode);
> >> }
> >> @@ -387,6 +396,7 @@ static const unsigned int virtbt_features[] = {
> >> VIRTIO_BT_F_VND_HCI,
> >> VIRTIO_BT_F_MSFT_EXT,
> >> VIRTIO_BT_F_AOSP_EXT,
> >> + VIRTIO_BT_F_CONFIG_V2,
> >> };
> >
> > So this introduces a new flag which must be checked when attempting to
> > config, right? But is this backward compatible? What happens if for
> > some reason the userspace doesn't use the new struct are we able to
> > detect that?
>
> Yes, it's backwards compatible.
>
> [q]Each virtio device offers all the features it understands. During
> device initialization, the driver reads this and tells the device the
> subset that it accepts. The only way to renegotiate is to reset the device.
> This allows for forwards and backwards compatibility: if the device is
> enhanced with a new feature bit, older drivers will not write that
> feature bit back to the device. Similarly, if a driver is enhanced with
> a feature that the device doesn’t support, it see the new feature is not
> offered.[/q]
>
> So, in our case:
>
> old device - new driver:
> The device does not offer VIRTIO_BT_F_CONFIG_V2 feature and uses the old
> configuration structure.
> The driver also uses the old configuration structure because
> VIRTIO_BT_F_CONFIG_V2 bit was not negotiated.
>
> new device - old driver:
> The device offers this bit, the driver reads it but cannot support it,
> so it does not write this bit back to the device during feature negotiation.
> The device verifies that this bit is not negotiated and continues to use
> the old configuration structure.
>
>
> I tested this patch, it
> a) works fine with a new device that supports VIRTIO_BT_F_CONFIG_V2.
> b) uses the old configuration structure when working with an old device.
> Our device does not offer the VIRTIO_BT_F_VND_HCI feature bit, so the
> driver does not tries to read unaligned "vendor" and "msft_opcode"
> fields and everything is fine.
> But, if the VIRTIO_BT_F_VND_HCI feature is set for the device for test
> purposes, our middle layer asserts unaligned accesses to the
> configuration space.
Great, thanks for the explanation.
> P.S. But, as Michael S. Tsirkin rightly stated, [q]Will a spec patch be
> forthcoming?[/q], this patch requires a specification update.
> I could not find any virtio_bt specification, do you have one?
> That would be great. Otherwise, would you mind if I try to write some
> initial draft?
Yep, I don't think we have one so feel free to start one, also while
at it we could perhaps attempt to write a tester for it so we can test
it using our CI, assuming virtio works with virtual devices created by
vhci driver.
> >> static struct virtio_driver virtbt_driver = {
> >> diff --git a/include/uapi/linux/virtio_bt.h b/include/uapi/linux/virtio_bt.h
> >> index a7bd48daa9a9..af798f4c9680 100644
> >> --- a/include/uapi/linux/virtio_bt.h
> >> +++ b/include/uapi/linux/virtio_bt.h
> >> @@ -9,6 +9,7 @@
> >> #define VIRTIO_BT_F_VND_HCI 0 /* Indicates vendor command support */
> >> #define VIRTIO_BT_F_MSFT_EXT 1 /* Indicates MSFT vendor support */
> >> #define VIRTIO_BT_F_AOSP_EXT 2 /* Indicates AOSP vendor support */
> >> +#define VIRTIO_BT_F_CONFIG_V2 3 /* Use second version configuration */
> >>
> >> enum virtio_bt_config_type {
> >> VIRTIO_BT_CONFIG_TYPE_PRIMARY = 0,
> >> @@ -28,4 +29,11 @@ struct virtio_bt_config {
> >> __u16 msft_opcode;
> >> } __attribute__((packed));
> >>
> >> +struct virtio_bt_config_v2 {
> >> + __u8 type;
> >> + __u8 alignment;
> >> + __u16 vendor;
> >> + __u16 msft_opcode;
> >> +};
> >> +
> >> #endif /* _UAPI_LINUX_VIRTIO_BT_H */
> >> --
> >> 2.37.2
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists