| Message ID | 20220127084754.GA25644@kili |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: target: iscsi: Use strcmp() instead of strncmp() | expand |
Sorry, I knee jerk react to "partial match", which was a red flag for me, so strncmp() is almost never used for "partial match" in my experience. :-) It's really that strncmp() should be used for buffer overflow protection, but in this case - it's not. Since, the ", 5" applies to the clearly visible "=All" that is both allocated and '\0' terminated. It should be against the remaining buffer size of text_ptr. So, removing the ", 5" leaves it equally as bad at buffer overflow protection as it was before. :-) On Thu, Jan 27, 2022 at 10:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > We want to match the whole string "=All" and this code does that, but > strncmp() is normally used for a partial match and it's more readable > to use strcmp(). > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/target/iscsi/iscsi_target.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > index 2c54c5d8412d..4534101a7376 100644 > --- a/drivers/target/iscsi/iscsi_target.c > +++ b/drivers/target/iscsi/iscsi_target.c > @@ -2213,7 +2213,7 @@ iscsit_process_text_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, > /* '=' confirmed in strncmp */ > text_ptr = strchr(text_in, '='); > BUG_ON(!text_ptr); > - if (!strncmp("=All", text_ptr, 5)) { > + if (!strcmp("=All", text_ptr)) { > cmd->cmd_flags |= ICF_SENDTARGETS_ALL; > } else if (!strncmp("=iqn.", text_ptr, 5) || > !strncmp("=eui.", text_ptr, 5)) { > -- > 2.20.1 >
On Sun, Jan 30, 2022 at 01:00:21PM -0500, Mark Mielke wrote: > Sorry, I knee jerk react to "partial match", which was a red flag for > me, so strncmp() is almost never used for "partial match" in my > experience. :-) > > It's really that strncmp() should be used for buffer overflow > protection, but in this case - it's not. Since, the ", 5" applies to > the clearly visible "=All" that is both allocated and '\0' terminated. > It should be against the remaining buffer size of text_ptr. So, > removing the ", 5" leaves it equally as bad at buffer overflow > protection as it was before. :-) This isn't really about my patch we all agree leaves things "as it was before". In this case, strncmp() was just copy and paste from the surrounding code and not used as an overflow check. What you're describing does exist but it's about 1% of use cases. There are about 2868 calls to strncmp() in the kernel. In my x86 allmodconfig only about 18 of them use strncmp() for buffer overflow checking. Then there are some real puzzlers which do: if (strncmp(variable, "foo", sizeof("foo")) == 0) { regards, dan carpenter
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 2c54c5d8412d..4534101a7376 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -2213,7 +2213,7 @@ iscsit_process_text_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, /* '=' confirmed in strncmp */ text_ptr = strchr(text_in, '='); BUG_ON(!text_ptr); - if (!strncmp("=All", text_ptr, 5)) { + if (!strcmp("=All", text_ptr)) { cmd->cmd_flags |= ICF_SENDTARGETS_ALL; } else if (!strncmp("=iqn.", text_ptr, 5) || !strncmp("=eui.", text_ptr, 5)) {
We want to match the whole string "=All" and this code does that, but strncmp() is normally used for a partial match and it's more readable to use strcmp(). Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)