netdev - Re: [PATCH v3] net: bluetooth: hci_sock: Use 'const u32 *' instead of 'void *' for 2nd parameter of hci_test

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: <54D6A729.1070905@sunrus.com.cn>
Date:	Sun, 08 Feb 2015 08:00:41 +0800
From:	Chen Gang S <gang.chen@...rus.com.cn>
To:	Joe Perches <joe@...ches.com>
CC:	David Laight <David.Laight@...LAB.COM>,
	Marcel Holtmann <marcel@...tmann.org>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v3] net: bluetooth: hci_sock: Use 'const u32 *' instead
 of 'void *' for 2nd parameter of hci_test_bit()

On 2/8/15 03:52, Joe Perches wrote:
> On Sat, 2015-02-07 at 21:24 +0800, Chen Gang S wrote:
>> hci_test_bit() does not modify 2nd parameter, so it is better to let it
>> be constant, or may cause build warning. The related warning (with
>> allmodconfig under xtensa):
>>
>>   net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
>>   net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
>>           &hci_sec_filter.ocf_mask[ogf])) &&
>>           ^
>>   net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
>>    static inline int hci_test_bit(int nr, void *addr)
>>                      ^
>>
>> hci_test_bit() always treats 2nd parameter is u32, and all callers also
>> know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
>> instead of 'void *'.
>>
>> C language treats the array function parameter as a pointer, so the
>> caller need not use '&' for the 2 demotion array, or it reports warning:
>> 'const unsigned int (*)[4]' is different with 'const unsigned int *'.
> 
> I still think you are possibly papering over potential bugs
> on big-endian 64 bit systems.
> 
> unsigned long vs u32.
> 
> How are the bits actually set?
> 

>From current usage of event_mask, "(u32 *) f->event_mask" is only for
event_mask data storage, not for calculation (always as "u32 *" for
calculation).

  [root@...alhost linux-next]# grep -rn "\<event_mask\>" include/net/bluetooth net/bluetooth
  include/net/bluetooth/hci_sock.h:51:	unsigned long event_mask[2];
  include/net/bluetooth/hci_sock.h:57:	__u32  event_mask[2];
  net/bluetooth/hci_sock.c:59:	__u32 event_mask[2];
  net/bluetooth/hci_sock.c:110:	if (!hci_test_bit(flt_event, (u32 *)&flt->event_mask))
  net/bluetooth/hci_sock.c:1041:			uf.event_mask[0] = *((u32 *) f->event_mask + 0);
  net/bluetooth/hci_sock.c:1042:			uf.event_mask[1] = *((u32 *) f->event_mask + 1);
  net/bluetooth/hci_sock.c:1053:			uf.event_mask[0] &= *((u32 *) hci_sec_filter.event_mask + 0);
  net/bluetooth/hci_sock.c:1054:			uf.event_mask[1] &= *((u32 *) hci_sec_filter.event_mask + 1);
  net/bluetooth/hci_sock.c:1062:			*((u32 *) f->event_mask + 0) = uf.event_mask[0];
  net/bluetooth/hci_sock.c:1063:			*((u32 *) f->event_mask + 1) = uf.event_mask[1];
  net/bluetooth/hci_sock.c:1124:			uf.event_mask[0] = *((u32 *) f->event_mask + 0);
  net/bluetooth/hci_sock.c:1125:			uf.event_mask[1] = *((u32 *) f->event_mask + 1);

Calculation is machine endian dependency, but event_mask is always as
"u32 *" for calculation, so there is no any type cast for calculation,
it is OK.

Storage is independent from machine endian, but it depends on machine
bits. In our case, 'unsigned long' array has enough space to accept u32
array, so there is no any data overwritten, it is OK.


By the way, I intended to remain event_mask as 'unsigned long' type,
because I am not quite sure whether it is also used by another modules
in kernel (or any other systems). May we change it to u32?

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html