[PATCH v2 0/3] firewire: fix minor issues

[PATCH v2 0/3] firewire: fix minor issues

[Takashi Sakamoto]

This second version of patchset is revised version of previous one[1] to
fix mistake of macro usage pointed out by reviewer[2].

As I note, they are not so urgent changes, thus I don't mind postponing
until next merge window.

[1] https://lore.kernel.org/alsa-devel/20220512111756.103008-1-o-takashi@sakamocchi.jp/
[2] https://lore.kernel.org/alsa-devel/87o7yvpf4t.wl-tiwai@suse.de/

Jiapeng Chong (1):
  firewire: convert sysfs sprintf/snprintf family to sysfs_emit

Lv Ruyi (1):
  firewire: Fix using uninitialized value

Minghao Chi (CGEL ZTE) (1):
  firewire: use struct_size over open coded arithmetic

 drivers/firewire/core-device.c      | 6 ++----
 drivers/firewire/core-transaction.c | 3 ++-
 2 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] firewire: convert sysfs sprintf/snprintf family to sysfs_emit

[Takashi Sakamoto]

From: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>

Fix the following coccicheck warning:

./drivers/firewire/core-device.c:375:8-16: WARNING: use scnprintf or
sprintf.

Reported-by: Abaci Robot<abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-device.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 90ed8fdaba75..adddd8c45d0c 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -372,8 +372,7 @@ static ssize_t rom_index_show(struct device *dev,
 	struct fw_device *device = fw_device(dev->parent);
 	struct fw_unit *unit = fw_unit(dev);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			(int)(unit->directory - device->config_rom));
+	return sysfs_emit(buf, "%td\n", unit->directory - device->config_rom);
 }
 
 static struct device_attribute fw_unit_attributes[] = {
@@ -403,8 +402,7 @@ static ssize_t guid_show(struct device *dev,
 	int ret;
 
 	down_read(&fw_device_rwsem);
-	ret = snprintf(buf, PAGE_SIZE, "0x%08x%08x\n",
-		       device->config_rom[3], device->config_rom[4]);
+	ret = sysfs_emit(buf, "0x%08x%08x\n", device->config_rom[3], device->config_rom[4]);
 	up_read(&fw_device_rwsem);
 
 	return ret;
-- 
2.34.1

Re: [PATCH v2 1/3] firewire: convert sysfs sprintf/snprintf family to sysfs_emit

[Takashi Iwai]

On Wed, 15 Jun 2022 14:15:03 +0200,
Takashi Sakamoto wrote:
> 
> From: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> 
> Fix the following coccicheck warning:
> 
> ./drivers/firewire/core-device.c:375:8-16: WARNING: use scnprintf or
> sprintf.
> 
> Reported-by: Abaci Robot<abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

I applied (only) this one now.


thanks,

Takashi

[PATCH v2 2/3] firewire: use struct_size over open coded arithmetic

[Takashi Sakamoto]

From: "Minghao Chi (CGEL ZTE)" <chi.minghao@zte.com.cn>

Replace zero-length array with flexible-array member and make use
of the struct_size() helper in kmalloc(). For example:

struct fw_request {
    ...
    u32 data[];
}

Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes.

(Revised by Takashi Sakamoto to fix the value of third argument.)

Signed-off-by: Minghao Chi (CGEL ZTE) <chi.minghao@zte.com.cn>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index af498d767702..4604a9d97fd1 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -779,7 +779,8 @@ static struct fw_request *allocate_request(struct fw_card *card,
 		return NULL;
 	}
 
-	request = kmalloc(sizeof(*request) + length, GFP_ATOMIC);
+	request = kmalloc(struct_size(request, data, length / sizeof(request->data[0])),
+			  GFP_ATOMIC);
 	if (request == NULL)
 		return NULL;
 
-- 
2.34.1

[PATCH v2 3/3] firewire: Fix using uninitialized value

[Takashi Sakamoto]

From: Lv Ruyi <lv.ruyi@zte.com.cn>

If data is null, request->data wouldn't be assigned value. It is random
value, but we use it in handle_exclusive_region_request() and
handle_fcp_region_request() later. Fix the bug by initializing it.

(Revised by Takashi Sakamoto to rebase to the previous patch.)

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 4604a9d97fd1..613aff624391 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -779,7 +779,7 @@ static struct fw_request *allocate_request(struct fw_card *card,
 		return NULL;
 	}
 
-	request = kmalloc(struct_size(request, data, length / sizeof(request->data[0])),
+	request = kzalloc(struct_size(request, data, length / sizeof(request->data[0])),
 			  GFP_ATOMIC);
 	if (request == NULL)
 		return NULL;
-- 
2.34.1

Re: [PATCH v2 0/3] firewire: fix minor issues

[Takashi Sakamoto]

Hi,

I realized that the second patch still includes a bug that shorter
buffer is allocated for block request than received length since the
computation is aligned to 4 without care of remainder.

Actually in the case of block request, the length is not necessarily
multiples of 4 and the packet payload has enough size of field with
padding to be aligned to 4, according to 1394 OHCI specification. In the
implementation of firewire-core driver, the field is copied without
the padding.

Please abandon them. I'm sorry to trouble you.


On Wed, Jun 15, 2022 at 09:15:02PM +0900, Takashi Sakamoto wrote:
> This second version of patchset is revised version of previous one[1] to
> fix mistake of macro usage pointed out by reviewer[2].
> 
> As I note, they are not so urgent changes, thus I don't mind postponing
> until next merge window.
> 
> [1] https://lore.kernel.org/alsa-devel/20220512111756.103008-1-o-takashi@sakamocchi.jp/
> [2] https://lore.kernel.org/alsa-devel/87o7yvpf4t.wl-tiwai@suse.de/
> 
> Jiapeng Chong (1):
>   firewire: convert sysfs sprintf/snprintf family to sysfs_emit
> 
> Lv Ruyi (1):
>   firewire: Fix using uninitialized value
> 
> Minghao Chi (CGEL ZTE) (1):
>   firewire: use struct_size over open coded arithmetic
> 
>  drivers/firewire/core-device.c      | 6 ++----
>  drivers/firewire/core-transaction.c | 3 ++-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> -- 
> 2.34.1


Reards

Takashi Sakamoto

Re: [PATCH v2 0/3] firewire: fix minor issues

[Takashi Iwai]

On Thu, 16 Jun 2022 02:21:42 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> I realized that the second patch still includes a bug that shorter
> buffer is allocated for block request than received length since the
> computation is aligned to 4 without care of remainder.
> 
> Actually in the case of block request, the length is not necessarily
> multiples of 4 and the packet payload has enough size of field with
> padding to be aligned to 4, according to 1394 OHCI specification. In the
> implementation of firewire-core driver, the field is copied without
> the padding.
> 
> Please abandon them. I'm sorry to trouble you.

So this implies that the type declaration of data[] rather looks
wrong?


Takashi

Re: [PATCH v2 0/3] firewire: fix minor issues

[Takashi Sakamoto]

Hi,

On Fri, Jun 17, 2022 at 10:42:51AM +0200, Takashi Iwai wrote:
> On Thu, 16 Jun 2022 02:21:42 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > I realized that the second patch still includes a bug that shorter
> > buffer is allocated for block request than received length since the
> > computation is aligned to 4 without care of remainder.
> > 
> > Actually in the case of block request, the length is not necessarily
> > multiples of 4 and the packet payload has enough size of field with
> > padding to be aligned to 4, according to 1394 OHCI specification. In the
> > implementation of firewire-core driver, the field is copied without
> > the padding.
> > 
> > Please abandon them. I'm sorry to trouble you.
> 
> So this implies that the type declaration of data[] rather looks
> wrong?

Your great insight.

Indeed, I can not find any code to dereference the array for u32
element. In all of cases, the 'struct fw_request.data' is passed losing
its pointer type (void *), then copied by the length in byte count. At
least, I can not find any warning or error at compiling the driver after
replacing the 'u32 []' with 'u8 []'.

Even if it were dereferenced, accessing over allocation boundary hardly
occurred since typical implementation of slab allocator maintains various
sizes of memory objects but multiples of 4.

It's possible to declare it with byte array, I think.


Thanks

Takashi Sakamoto