[Qemu-trivial] [PATCH for-2.5] bt: fix two struct sizes

[Qemu-trivial] [PATCH for-2.5] bt: fix two struct sizes

[Paolo Bonzini]

See http://permalink.gmane.org/gmane.linux.bluez.kernel/36505.  For historical
reasons these do not use sizeof, and Coverity caught a mistake in
EVT_ENCRYPT_CHANGE_SIZE.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/bt.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/bt.h b/include/hw/bt.h
index cb2a7e6..bbea104 100644
--- a/include/hw/bt.h
+++ b/include/hw/bt.h
@@ -1266,7 +1266,7 @@ typedef struct {
     uint8_t	status;
     uint16_t	handle;
 } QEMU_PACKED reset_failed_contact_counter_rp;
-#define RESET_FAILED_CONTACT_COUNTER_RP_SIZE 4
+#define RESET_FAILED_CONTACT_COUNTER_RP_SIZE 3
 
 #define OCF_READ_LINK_QUALITY		0x0003
 typedef struct {
@@ -1381,7 +1381,7 @@ typedef struct {
     uint16_t	handle;
     uint8_t	encrypt;
 } QEMU_PACKED evt_encrypt_change;
-#define EVT_ENCRYPT_CHANGE_SIZE 5
+#define EVT_ENCRYPT_CHANGE_SIZE 4
 
 #define EVT_CHANGE_CONN_LINK_KEY_COMPLETE	0x09
 typedef struct {
-- 
2.5.0

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> See http://permalink.gmane.org/gmane.linux.bluez.kernel/36505.  For historical
> reasons these do not use sizeof, and Coverity caught a mistake in
> EVT_ENCRYPT_CHANGE_SIZE.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/hw/bt.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/bt.h b/include/hw/bt.h
> index cb2a7e6..bbea104 100644
> --- a/include/hw/bt.h
> +++ b/include/hw/bt.h
> @@ -1266,7 +1266,7 @@ typedef struct {
>      uint8_t	status;
>      uint16_t	handle;
>  } QEMU_PACKED reset_failed_contact_counter_rp;
> -#define RESET_FAILED_CONTACT_COUNTER_RP_SIZE 4
> +#define RESET_FAILED_CONTACT_COUNTER_RP_SIZE 3
>  
>  #define OCF_READ_LINK_QUALITY		0x0003
>  typedef struct {
> @@ -1381,7 +1381,7 @@ typedef struct {
>      uint16_t	handle;
>      uint8_t	encrypt;
>  } QEMU_PACKED evt_encrypt_change;
> -#define EVT_ENCRYPT_CHANGE_SIZE 5
> +#define EVT_ENCRYPT_CHANGE_SIZE 4
>  
>  #define EVT_CHANGE_CONN_LINK_KEY_COMPLETE	0x09
>  typedef struct {

Yuck!

Since you also fix RESET_FAILED_CONTACT_COUNTER_RP_SIZE, I assume you
checked them all.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Paolo Bonzini]

On 27/11/2015 15:39, Markus Armbruster wrote:
> Since you also fix RESET_FAILED_CONTACT_COUNTER_RP_SIZE, I assume you
> checked them all.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

No, I just copied the upstream bluez patch.

Paolo

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/11/2015 15:39, Markus Armbruster wrote:
>> Since you also fix RESET_FAILED_CONTACT_COUNTER_RP_SIZE, I assume you
>> checked them all.
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> No, I just copied the upstream bluez patch.

At least CREATE_CONN_CANCEL_CP_SIZE is also wrong.

Any particular reason not to mass-convert to sizeof and call it a day?

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Paolo Bonzini]

On 27/11/2015 16:16, Markus Armbruster wrote:
>>> >> Since you also fix RESET_FAILED_CONTACT_COUNTER_RP_SIZE, I assume you
>>> >> checked them all.
>>> >> 
>>> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > No, I just copied the upstream bluez patch.
> At least CREATE_CONN_CANCEL_CP_SIZE is also wrong.
> 
> Any particular reason not to mass-convert to sizeof and call it a day?

No particular reason, but it is bluetooth after all. :)

Paolo

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/11/2015 16:16, Markus Armbruster wrote:
>>>> >> Since you also fix RESET_FAILED_CONTACT_COUNTER_RP_SIZE, I assume you
>>>> >> checked them all.
>>>> >> 
>>>> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> >
>>> > No, I just copied the upstream bluez patch.
>> At least CREATE_CONN_CANCEL_CP_SIZE is also wrong.
>> 
>> Any particular reason not to mass-convert to sizeof and call it a day?
>
> No particular reason, but it is bluetooth after all. :)

Okay, no need to say more.

A bit of Emacs wizardry produced a test program, which coughed up three
more:

type                            sizeof()  _SIZE
create_conn_cancel_cp                  7      6
reset_failed_contact_counter_rp        3      4
read_link_quality_cp                   2      4
inquiry_info                          15     14
evt_encrypt_change                     4      5

I'll send a patch.

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 27/11/2015 16:16, Markus Armbruster wrote:
>>>>> >> Since you also fix RESET_FAILED_CONTACT_COUNTER_RP_SIZE, I assume you
>>>>> >> checked them all.
>>>>> >> 
>>>>> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Retracted.

>>>> > No, I just copied the upstream bluez patch.
>>> At least CREATE_CONN_CANCEL_CP_SIZE is also wrong.
>>> 
>>> Any particular reason not to mass-convert to sizeof and call it a day?
>>
>> No particular reason, but it is bluetooth after all. :)
>
> Okay, no need to say more.
>
> A bit of Emacs wizardry produced a test program, which coughed up three
> more:
>
> type                            sizeof()  _SIZE
> create_conn_cancel_cp                  7      6

Unused in QEMU.

The closest match I can find in the kernel's current
include/net/bluetooth/hci.h is

    #define HCI_OP_CREATE_CONN_CANCEL       0x0408
    struct hci_cp_create_conn_cancel {
            bdaddr_t bdaddr;
    } __packed;

Size 6, QEMU has an additional first member that brings it to size 7.
Pffft, I'm not going to touch this crap.

> reset_failed_contact_counter_rp        3      4

Unused in QEMU.  Can't match to the kernel's header.  You changed this
one.  I wouldn't touch it.

> read_link_quality_cp                   2      4

Unused in QEMU.  Can't match to the kernel's header.  I'm not going to
touch it.

> inquiry_info                          15     14

Used in bt_hci_inquiry_result_standard().  I figure bt_hci_event() will
leave the last byte uninitialized.

Kernel has

    struct inquiry_info {
            bdaddr_t bdaddr;
            __u8     pscan_rep_mode;
            __u8     pscan_period_mode;
            __u8     pscan_mode;
            __u8     dev_class[3];
            __le16   clock_offset;
    } __packed;

Size 14, QEMU has an additional first member that brings it to size 15.
I'm not going to touch it.

> evt_encrypt_change                     4      5

Used in bt_hci_event_encrypt_change().  I figure it makes bt_hci_event()
overrun the destination by one byte.

Kernel has

    struct hci_ev_encrypt_change {
            __u8     status;
            __le16   handle;
            __u8     encrypt;
    } __packed;

You changed this one.  Plausible, but I don't want to have my R-by on it
all the same.

> I'll send a patch.

No, I won't.  The only patch I could be persuaded to send would drop the
whole thing with prejudice.

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Paolo Bonzini]

On 27/11/2015 18:08, Markus Armbruster wrote:
>> > evt_encrypt_change                     4      5
> Used in bt_hci_event_encrypt_change().  I figure it makes bt_hci_event()
> overrun the destination by one byte.

Yes, and Coverity complains.

> Kernel has
> 
>     struct hci_ev_encrypt_change {
>             __u8     status;
>             __le16   handle;
>             __u8     encrypt;
>     } __packed;
> 
> You changed this one.  Plausible, but I don't want to have my R-by on it
> all the same.

Shall I proceed with this patch, just without R-by?  Or only modify the
one where Coverity complains?  I picked this one because it matches a
bluez patch.

Paolo

Re: [Qemu-trivial] [Qemu-devel] [PATCH for-2.5] bt: fix two struct sizes

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/11/2015 18:08, Markus Armbruster wrote:
>>> > evt_encrypt_change                     4      5
>> Used in bt_hci_event_encrypt_change().  I figure it makes bt_hci_event()
>> overrun the destination by one byte.
>
> Yes, and Coverity complains.
>
>> Kernel has
>> 
>>     struct hci_ev_encrypt_change {
>>             __u8     status;
>>             __le16   handle;
>>             __u8     encrypt;
>>     } __packed;
>> 
>> You changed this one.  Plausible, but I don't want to have my R-by on it
>> all the same.
>
> Shall I proceed with this patch, just without R-by?  Or only modify the
> one where Coverity complains?  I picked this one because it matches a
> bluez patch.

Yes, that seems to make the most sense.  If you feel like it, add a
sentence or two on the ones you don't fix to the commit message.