Message ID | 1448626105-29540-1-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
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>
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
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?
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
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.
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.
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
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.
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 {
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(-)