* [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem()
@ 2023-09-04 19:54 Christophe JAILLET
2023-09-05 5:04 ` Damien Le Moal
2023-09-11 6:14 ` Damien Le Moal
0 siblings, 2 replies; 6+ messages in thread
From: Christophe JAILLET @ 2023-09-04 19:54 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-ide
snprintf() returns the "number of characters which *would* be generated for
the given input", not the size *really* generated.
In order to avoid too large values for 'o' (and potential negative values
for "sizeof(linebuf) o") use scnprintf() instead of snprintf().
Note that given the "w < 4" in the for loop, the buffer can NOT
overflow, but using the *right* function is always better.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/ata/sata_mv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index d105db5c7d81..45e48d653c60 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1255,8 +1255,8 @@ static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes)
for (b = 0; b < bytes; ) {
for (w = 0, o = 0; b < bytes && w < 4; w++) {
- o += snprintf(linebuf + o, sizeof(linebuf) - o,
- "%08x ", readl(start + b));
+ o += scnprintf(linebuf + o, sizeof(linebuf) - o,
+ "%08x ", readl(start + b));
b += sizeof(u32);
}
dev_dbg(dev, "%s: %p: %s\n",
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem()
2023-09-04 19:54 [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem() Christophe JAILLET
@ 2023-09-05 5:04 ` Damien Le Moal
2023-09-05 15:28 ` Christophe JAILLET
2023-09-11 6:14 ` Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2023-09-05 5:04 UTC (permalink / raw)
To: Christophe JAILLET; +Cc: linux-kernel, kernel-janitors, linux-ide
On 9/5/23 04:54, Christophe JAILLET wrote:
> snprintf() returns the "number of characters which *would* be generated for
> the given input", not the size *really* generated.
>
> In order to avoid too large values for 'o' (and potential negative values
> for "sizeof(linebuf) o") use scnprintf() instead of snprintf().
>
> Note that given the "w < 4" in the for loop, the buffer can NOT
> overflow, but using the *right* function is always better.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Doesn't this need Fixes and CC stable tags ?
> ---
> drivers/ata/sata_mv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index d105db5c7d81..45e48d653c60 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -1255,8 +1255,8 @@ static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes)
>
> for (b = 0; b < bytes; ) {
> for (w = 0, o = 0; b < bytes && w < 4; w++) {
> - o += snprintf(linebuf + o, sizeof(linebuf) - o,
> - "%08x ", readl(start + b));
> + o += scnprintf(linebuf + o, sizeof(linebuf) - o,
> + "%08x ", readl(start + b));
> b += sizeof(u32);
> }
> dev_dbg(dev, "%s: %p: %s\n",
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem()
2023-09-05 5:04 ` Damien Le Moal
@ 2023-09-05 15:28 ` Christophe JAILLET
2023-09-06 1:11 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Christophe JAILLET @ 2023-09-05 15:28 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-kernel, kernel-janitors, linux-ide
Le 05/09/2023 à 07:04, Damien Le Moal a écrit :
> On 9/5/23 04:54, Christophe JAILLET wrote:
>> snprintf() returns the "number of characters which *would* be generated for
>> the given input", not the size *really* generated.
>>
>> In order to avoid too large values for 'o' (and potential negative values
>> for "sizeof(linebuf) o") use scnprintf() instead of snprintf().
>>
>> Note that given the "w < 4" in the for loop, the buffer can NOT
>> overflow, but using the *right* function is always better.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> Doesn't this need Fixes and CC stable tags ?
I don't think so.
As said in the commit message :
Note that given the "w < 4" in the for loop, the buffer can NOT
overflow, but using the *right* function is always better.
linebuf is 38 chars.
In each iteration, we write 9 bytes + NULL.
We write only 4 elements per line (because of w < 4), so 9 * 4 + 1 = 37
bytes are needed.
9 is for %08x<space>
It can't overflow.
Moreover, it is really unlikely that the size of linebuf or the number
of elements on each line change in a stable kernel.
So, from my POV, this patch is more a clean-up than anything else.
I would even agree that it is maybe not even needed. But should someone
cut'n'paste it one day, then using the correct function could maybe help
him.
CJ
>
>> ---
>> drivers/ata/sata_mv.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>> index d105db5c7d81..45e48d653c60 100644
>> --- a/drivers/ata/sata_mv.c
>> +++ b/drivers/ata/sata_mv.c
>> @@ -1255,8 +1255,8 @@ static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes)
>>
>> for (b = 0; b < bytes; ) {
>> for (w = 0, o = 0; b < bytes && w < 4; w++) {
>> - o += snprintf(linebuf + o, sizeof(linebuf) - o,
>> - "%08x ", readl(start + b));
>> + o += scnprintf(linebuf + o, sizeof(linebuf) - o,
>> + "%08x ", readl(start + b));
>> b += sizeof(u32);
>> }
>> dev_dbg(dev, "%s: %p: %s\n",
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem()
2023-09-05 15:28 ` Christophe JAILLET
@ 2023-09-06 1:11 ` Damien Le Moal
2023-09-08 5:18 ` Christophe JAILLET
0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2023-09-06 1:11 UTC (permalink / raw)
To: Christophe JAILLET; +Cc: linux-kernel, kernel-janitors, linux-ide
On 9/6/23 00:28, Christophe JAILLET wrote:
>
>
> Le 05/09/2023 à 07:04, Damien Le Moal a écrit :
>> On 9/5/23 04:54, Christophe JAILLET wrote:
>>> snprintf() returns the "number of characters which *would* be generated for
>>> the given input", not the size *really* generated.
>>>
>>> In order to avoid too large values for 'o' (and potential negative values
>>> for "sizeof(linebuf) o") use scnprintf() instead of snprintf().
>>>
>>> Note that given the "w < 4" in the for loop, the buffer can NOT
>>> overflow, but using the *right* function is always better.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>
>> Doesn't this need Fixes and CC stable tags ?
>
> I don't think so.
> As said in the commit message :
> Note that given the "w < 4" in the for loop, the buffer can NOT
> overflow, but using the *right* function is always better.
>
> linebuf is 38 chars.
> In each iteration, we write 9 bytes + NULL.
> We write only 4 elements per line (because of w < 4), so 9 * 4 + 1 = 37
> bytes are needed.
> 9 is for %08x<space>
>
> It can't overflow.
> Moreover, it is really unlikely that the size of linebuf or the number
> of elements on each line change in a stable kernel.
>
> So, from my POV, this patch is more a clean-up than anything else.
>
> I would even agree that it is maybe not even needed. But should someone
> cut'n'paste it one day, then using the correct function could maybe help
> him.
OK. Fine. But then maybe the patch title should be something like "Improve
string length computation in mv_dump_mem()" as the "Fix" word you used seems to
be somewhat misleading. With the patch title as is, the stable bot will likely
pick up that patch for stable. Fine with me, unless you see an issue with that.
>
> CJ
>
>>
>>> ---
>>> drivers/ata/sata_mv.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>>> index d105db5c7d81..45e48d653c60 100644
>>> --- a/drivers/ata/sata_mv.c
>>> +++ b/drivers/ata/sata_mv.c
>>> @@ -1255,8 +1255,8 @@ static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes)
>>>
>>> for (b = 0; b < bytes; ) {
>>> for (w = 0, o = 0; b < bytes && w < 4; w++) {
>>> - o += snprintf(linebuf + o, sizeof(linebuf) - o,
>>> - "%08x ", readl(start + b));
>>> + o += scnprintf(linebuf + o, sizeof(linebuf) - o,
>>> + "%08x ", readl(start + b));
>>> b += sizeof(u32);
>>> }
>>> dev_dbg(dev, "%s: %p: %s\n",
>>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem()
2023-09-06 1:11 ` Damien Le Moal
@ 2023-09-08 5:18 ` Christophe JAILLET
0 siblings, 0 replies; 6+ messages in thread
From: Christophe JAILLET @ 2023-09-08 5:18 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-kernel, kernel-janitors, linux-ide
Le 06/09/2023 à 03:11, Damien Le Moal a écrit :
> On 9/6/23 00:28, Christophe JAILLET wrote:
>>
>>
>> Le 05/09/2023 à 07:04, Damien Le Moal a écrit :
>>> On 9/5/23 04:54, Christophe JAILLET wrote:
>>>> snprintf() returns the "number of characters which *would* be generated for
>>>> the given input", not the size *really* generated.
>>>>
>>>> In order to avoid too large values for 'o' (and potential negative values
>>>> for "sizeof(linebuf) o") use scnprintf() instead of snprintf().
>>>>
>>>> Note that given the "w < 4" in the for loop, the buffer can NOT
>>>> overflow, but using the *right* function is always better.
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>
>>> Doesn't this need Fixes and CC stable tags ?
>>
>> I don't think so.
>> As said in the commit message :
>> Note that given the "w < 4" in the for loop, the buffer can NOT
>> overflow, but using the *right* function is always better.
>>
>> linebuf is 38 chars.
>> In each iteration, we write 9 bytes + NULL.
>> We write only 4 elements per line (because of w < 4), so 9 * 4 + 1 = 37
>> bytes are needed.
>> 9 is for %08x<space>
>>
>> It can't overflow.
>> Moreover, it is really unlikely that the size of linebuf or the number
>> of elements on each line change in a stable kernel.
>>
>> So, from my POV, this patch is more a clean-up than anything else.
>>
>> I would even agree that it is maybe not even needed. But should someone
>> cut'n'paste it one day, then using the correct function could maybe help
>> him.
>
> OK. Fine. But then maybe the patch title should be something like "Improve
> string length computation in mv_dump_mem()" as the "Fix" word you used seems to
> be somewhat misleading. With the patch title as is, the stable bot will likely
> pick up that patch for stable. Fine with me, unless you see an issue with that.
Hi,
up to you to tweak it if desired.
My POV is that it *fixes* the length calculation, but having it "wrong"
is harmless.
Should it trigger a backport, it wouldn't be a real issue either. And we
can still ask to remove it from the backport list when notified.
And as said, leaving things as-is looks also fine to me.
I let you choose the best option.
CJ
>
>>
>> CJ
>>
>>>
>>>> ---
>>>> drivers/ata/sata_mv.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>>>> index d105db5c7d81..45e48d653c60 100644
>>>> --- a/drivers/ata/sata_mv.c
>>>> +++ b/drivers/ata/sata_mv.c
>>>> @@ -1255,8 +1255,8 @@ static void mv_dump_mem(struct device *dev, void __iomem *start, unsigned bytes)
>>>>
>>>> for (b = 0; b < bytes; ) {
>>>> for (w = 0, o = 0; b < bytes && w < 4; w++) {
>>>> - o += snprintf(linebuf + o, sizeof(linebuf) - o,
>>>> - "%08x ", readl(start + b));
>>>> + o += scnprintf(linebuf + o, sizeof(linebuf) - o,
>>>> + "%08x ", readl(start + b));
>>>> b += sizeof(u32);
>>>> }
>>>> dev_dbg(dev, "%s: %p: %s\n",
>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem()
2023-09-04 19:54 [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem() Christophe JAILLET
2023-09-05 5:04 ` Damien Le Moal
@ 2023-09-11 6:14 ` Damien Le Moal
1 sibling, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2023-09-11 6:14 UTC (permalink / raw)
To: Christophe JAILLET; +Cc: linux-kernel, kernel-janitors, linux-ide
On 9/5/23 04:54, Christophe JAILLET wrote:
> snprintf() returns the "number of characters which *would* be generated for
> the given input", not the size *really* generated.
>
> In order to avoid too large values for 'o' (and potential negative values
> for "sizeof(linebuf) o") use scnprintf() instead of snprintf().
>
> Note that given the "w < 4" in the for loop, the buffer can NOT
> overflow, but using the *right* function is always better.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Applied to for-6.6-fixes. Thanks !
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-11 6:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-04 19:54 [PATCH] ata: sata_mv: Fix incorrect string length computation in mv_dump_mem() Christophe JAILLET
2023-09-05 5:04 ` Damien Le Moal
2023-09-05 15:28 ` Christophe JAILLET
2023-09-06 1:11 ` Damien Le Moal
2023-09-08 5:18 ` Christophe JAILLET
2023-09-11 6:14 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®