Hi Mike,
On Wed, 2 Oct 2019 12:09:38 -0500, Mike Christie wrote:
> On 09/12/2019 04:55 AM, David Disseldorp wrote:
> > strncmp is currently used for "SendTargets" key and "All" value matching
> > without checking for trailing garbage. This means that Text request PDUs
> > with garbage such as "SendTargetsPlease=All" and "SendTargets=Alle" are
> > processed successfully as if they were "SendTargets=All" requests.
> >
> > Signed-off-by: David Disseldorp <ddiss@xxxxxxx>
> > ---
> > drivers/target/iscsi/iscsi_target.c | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> > index d19e051f2bc2..09e55ea0bf5d 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
> > @@ -2189,24 +2189,22 @@ iscsit_process_text_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> > }
> > goto empty_sendtargets;
> > }
> > - if (strncmp("SendTargets", text_in, 11) != 0) {
> > + if (strncmp("SendTargets=", text_in, 12) != 0) {
> > pr_err("Received Text Data that is not"
> > " SendTargets, cannot continue.\n");
> > goto reject;
> > }
> > + /* '=' confirmed in strncmp */
> > text_ptr = strchr(text_in, '=');
> > - if (!text_ptr) {
> > - pr_err("No \"=\" separator found in Text Data,"
> > - " cannot continue.\n");
> > - goto reject;
> > - }
> > - if (!strncmp("=All", text_ptr, 4)) {
> > + BUG_ON(!text_ptr);
> > + if (!strncmp("=All", text_ptr, 5)) {
>
> Why is the count 5 now?
To ensure that the compare includes the null terminator, rejecting
trailing garbage (e.g. "SendTargets=AllGarbage"). strcmp() would also be
an option.
> Did the ones below need to be increased too then?
>
> > cmd->cmd_flags |= ICF_SENDTARGETS_ALL;
> > } else if (!strncmp("=iqn.", text_ptr, 5) ||
> > !strncmp("=eui.", text_ptr, 5)) {
> > cmd->cmd_flags |= ICF_SENDTARGETS_SINGLE;
No, in this case we only want to check for the valid prefix. The full
string is later compared against existing target names.
Cheers, David
