linux-kernel - Re: [PATCH] target: iscsi: iscsi_target_tpg.c: Cleaning up possible size overwriting in conjunction with sprintf

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] [day] [month] [year] [list]

Message-ID: <CAFo99gY9QBxwj_nM4xddpMze7hbY5dOu1f6_ZvErCrzwOo4LHg@mail.gmail.com>
Date:	Sun, 12 Oct 2014 21:32:04 +0200
From:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
To:	Joe Perches <joe@...ches.com>
Cc:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>,
	Sagi Grimberg <sagig@...lanox.com>,
	Andy Grover <agrover@...hat.com>,
	Thomas Glanzmann <thomas@...nzmann.de>,
	Christophe Vu-Brugier <cvubrugier@...oo.fr>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	target-devel@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] target: iscsi: iscsi_target_tpg.c: Cleaning up possible
 size overwriting in conjunction with sprintf

2014-10-12 20:18 GMT+02:00 Joe Perches <joe@...ches.com>:
> On Sun, 2014-10-12 at 19:55 +0200, Rickard Strandqvist wrote:
>> Changed same snprintf and sprintf to strlcpy and strlcat.
>> This will guarantee that the string size is not overwritten,
>> and they are significantly faster than sprintf also.
>
> I don't disagree with the change, but I think the changelog is
> misleading.
>
> strlcpy(ptr, ...) then strlcat(ptr, ...) does not use the known
> length of the first copied string and is generally not faster than
> ptr += snprintf(ptr, ...) then sprintf(ptr,...)
>
> And a couple slightly unrelated bits:
>
>> diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
>> index c3cb5c1..6fc8bfe 100644
>> --- a/drivers/target/iscsi/iscsi_target_tpg.c
>> +++ b/drivers/target/iscsi/iscsi_target_tpg.c
>> @@ -608,7 +608,6 @@ int iscsit_tpg_set_initiator_node_queue_depth(
>>  int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
>
> This function could be made static and the prototype removed from
> the .h file if it is moved above its one caller in this file.
>
>>  {
>>       unsigned char buf1[256], buf2[256], *none = NULL;
>
> Moderately large stack use here.
>



Hi

Can not agree with you about speed.
But regardless of that, it is ironically precisely the behavior you
describe that is a part of the problem.
Form the man page for snprintf
"If the output was truncated due to this limit then the return value
is the number of characters (excluding the terminating null byte)
which would have been written to the final string if enough space had
been available"

Switching to static funktion sounds good.
And less use of static memory, anyone have a more reasonable idea of a size?


Kind regards
Rickard Strandqvist
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/