| Message ID | 20210807134651.245436-1-islituo@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: target: pscsi: Fix possible null-pointer dereference in pscsi_complete_cmd() | expand |
On 07.08.21 15:46, Tuo Li wrote: > The return value of transport_kmap_data_sg() is assigned to the variable > buf: > buf = transport_kmap_data_sg(cmd); > > And then it is checked: > if (!buf) { > > This indicates that buf can be NULL. However, it is dereferenced in the > following statements: > if (!(buf[3] & 0x80)) > buf[3] |= 0x80; > if (!(buf[2] & 0x80)) > buf[2] |= 0x80; > > To fix these possible null-pointer dereferences, dereference buf only when > it is not NULL. > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Tuo Li <islituo@gmail.com> > --- > drivers/target/target_core_pscsi.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c > index 2629d2ef3970..560815729182 100644 > --- a/drivers/target/target_core_pscsi.c > +++ b/drivers/target/target_core_pscsi.c > @@ -620,14 +620,14 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status, > buf = transport_kmap_data_sg(cmd); > if (!buf) { > ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ > - } > - > - if (cdb[0] == MODE_SENSE_10) { > - if (!(buf[3] & 0x80)) > - buf[3] |= 0x80; > } else { > - if (!(buf[2] & 0x80)) > - buf[2] |= 0x80; > + if (cdb[0] == MODE_SENSE_10) { > + if (!(buf[3] & 0x80)) > + buf[3] |= 0x80; > + } else { > + if (!(buf[2] & 0x80)) > + buf[2] |= 0x80; > + } > } > > transport_kunmap_data_sg(cmd); > I'm wondering whether we should better put the transport_kunmap_data_sg into the else-branch of the if (!buf)? AFAICS, calling it after transport_kmap_data_sg failed does not cause problems, but I feel it would be cleaner. Otherwise it looks good to me.
Thanks for your feedback. We will prepare a V2 patch and put the transport_kunmap_data_sg() into the else-branch of the if (!buf). Best wishes, Tuo Li On 2021/8/9 18:36, Bodo Stroesser wrote: > On 07.08.21 15:46, Tuo Li wrote: >> The return value of transport_kmap_data_sg() is assigned to the variable >> buf: >> buf = transport_kmap_data_sg(cmd); >> >> And then it is checked: >> if (!buf) { >> >> This indicates that buf can be NULL. However, it is dereferenced in the >> following statements: >> if (!(buf[3] & 0x80)) >> buf[3] |= 0x80; >> if (!(buf[2] & 0x80)) >> buf[2] |= 0x80; >> >> To fix these possible null-pointer dereferences, dereference buf only >> when >> it is not NULL. >> >> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >> Signed-off-by: Tuo Li <islituo@gmail.com> >> --- >> drivers/target/target_core_pscsi.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/target/target_core_pscsi.c >> b/drivers/target/target_core_pscsi.c >> index 2629d2ef3970..560815729182 100644 >> --- a/drivers/target/target_core_pscsi.c >> +++ b/drivers/target/target_core_pscsi.c >> @@ -620,14 +620,14 @@ static void pscsi_complete_cmd(struct se_cmd >> *cmd, u8 scsi_status, >> buf = transport_kmap_data_sg(cmd); >> if (!buf) { >> ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ >> - } >> - >> - if (cdb[0] == MODE_SENSE_10) { >> - if (!(buf[3] & 0x80)) >> - buf[3] |= 0x80; >> } else { >> - if (!(buf[2] & 0x80)) >> - buf[2] |= 0x80; >> + if (cdb[0] == MODE_SENSE_10) { >> + if (!(buf[3] & 0x80)) >> + buf[3] |= 0x80; >> + } else { >> + if (!(buf[2] & 0x80)) >> + buf[2] |= 0x80; >> + } >> } >> transport_kunmap_data_sg(cmd); >> > > I'm wondering whether we should better put the > transport_kunmap_data_sg into the else-branch of the if (!buf)? > AFAICS, calling it after transport_kmap_data_sg failed does not > cause problems, but I feel it would be cleaner. > > Otherwise it looks good to me.
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 2629d2ef3970..560815729182 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -620,14 +620,14 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status, buf = transport_kmap_data_sg(cmd); if (!buf) { ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */ - } - - if (cdb[0] == MODE_SENSE_10) { - if (!(buf[3] & 0x80)) - buf[3] |= 0x80; } else { - if (!(buf[2] & 0x80)) - buf[2] |= 0x80; + if (cdb[0] == MODE_SENSE_10) { + if (!(buf[3] & 0x80)) + buf[3] |= 0x80; + } else { + if (!(buf[2] & 0x80)) + buf[2] |= 0x80; + } } transport_kunmap_data_sg(cmd);
The return value of transport_kmap_data_sg() is assigned to the variable buf: buf = transport_kmap_data_sg(cmd); And then it is checked: if (!buf) { This indicates that buf can be NULL. However, it is dereferenced in the following statements: if (!(buf[3] & 0x80)) buf[3] |= 0x80; if (!(buf[2] & 0x80)) buf[2] |= 0x80; To fix these possible null-pointer dereferences, dereference buf only when it is not NULL. Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Signed-off-by: Tuo Li <islituo@gmail.com> --- drivers/target/target_core_pscsi.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)