| Message ID | 20231130204056.it.978-kees@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] scsi: zfcp: Replace strlcpy() with strscpy() | expand |
Hello Kees, Martin, James, On Thu, Nov 30, 2023 at 12:41:00PM -0800, Kees Cook wrote: > strlcpy() reads the entire source buffer first. This read may exceed > the destination size limit. This is both inefficient and can lead > to linear read overflows if a source string is not NUL-terminated[1]. > Additionally, it returns the size of the source string, not the > resulting size of the destination string. In an effort to remove strlcpy() > completely[2], replace strlcpy() here with strscpy(). > > Overflow should be impossible here, but actually check for buffer sizes > being identical with BUILD_BUG_ON(), and include a run-time check as > well. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1] > Link: https://github.com/KSPP/linux/issues/89 [2] > --- > drivers/s390/scsi/zfcp_fc.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c > index 4f0d0e55f0d4..d6516ab00437 100644 > --- a/drivers/s390/scsi/zfcp_fc.c > +++ b/drivers/s390/scsi/zfcp_fc.c > @@ -900,8 +900,19 @@ static void zfcp_fc_rspn(struct zfcp_adapter *adapter, > zfcp_fc_ct_ns_init(&rspn_req->ct_hdr, FC_NS_RSPN_ID, > FC_SYMBOLIC_NAME_SIZE); > hton24(rspn_req->rspn.fr_fid.fp_fid, fc_host_port_id(shost)); > - len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost), > - FC_SYMBOLIC_NAME_SIZE); > + > + BUILD_BUG_ON(sizeof(rspn_req->name) != > + sizeof(fc_host_symbolic_name(shost))); > + BUILD_BUG_ON(sizeof(rspn_req->name) != > + type_max(typeof(rspn_req->rspn.fr_name_len)) + 1); > + len = strscpy(rspn_req->name, fc_host_symbolic_name(shost), > + sizeof(rspn_req->name)); > + /* > + * It should be impossible for this to truncate (see BUILD_BUG_ON() > + * above), but be robust anyway. > + */ > + if (WARN_ON(len < 0)) > + len = sizeof(rspn_req->name) - 1; Looks good to me. Acked-by: Benjamin Block <bblock@linux.ibm.com> > rspn_req->rspn.fr_name_len = len; Martin, James, can you please pick this up for the v6.8 queue?
Kees, > strlcpy() reads the entire source buffer first. This read may exceed > the destination size limit. This is both inefficient and can lead to > linear read overflows if a source string is not NUL-terminated[1]. > Additionally, it returns the size of the source string, not the > resulting size of the destination string. In an effort to remove > strlcpy() completely[2], replace strlcpy() here with strscpy(). Applied to 6.8/scsi-staging, thanks!
On Thu, 30 Nov 2023 12:41:00 -0800, Kees Cook wrote: > strlcpy() reads the entire source buffer first. This read may exceed > the destination size limit. This is both inefficient and can lead > to linear read overflows if a source string is not NUL-terminated[1]. > Additionally, it returns the size of the source string, not the > resulting size of the destination string. In an effort to remove strlcpy() > completely[2], replace strlcpy() here with strscpy(). > > [...] Applied to 6.8/scsi-queue, thanks! [1/1] scsi: zfcp: Replace strlcpy() with strscpy() https://git.kernel.org/mkp/scsi/c/0d224b1088af
diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c index 4f0d0e55f0d4..d6516ab00437 100644 --- a/drivers/s390/scsi/zfcp_fc.c +++ b/drivers/s390/scsi/zfcp_fc.c @@ -900,8 +900,19 @@ static void zfcp_fc_rspn(struct zfcp_adapter *adapter, zfcp_fc_ct_ns_init(&rspn_req->ct_hdr, FC_NS_RSPN_ID, FC_SYMBOLIC_NAME_SIZE); hton24(rspn_req->rspn.fr_fid.fp_fid, fc_host_port_id(shost)); - len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost), - FC_SYMBOLIC_NAME_SIZE); + + BUILD_BUG_ON(sizeof(rspn_req->name) != + sizeof(fc_host_symbolic_name(shost))); + BUILD_BUG_ON(sizeof(rspn_req->name) != + type_max(typeof(rspn_req->rspn.fr_name_len)) + 1); + len = strscpy(rspn_req->name, fc_host_symbolic_name(shost), + sizeof(rspn_req->name)); + /* + * It should be impossible for this to truncate (see BUILD_BUG_ON() + * above), but be robust anyway. + */ + if (WARN_ON(len < 0)) + len = sizeof(rspn_req->name) - 1; rspn_req->rspn.fr_name_len = len; sg_init_one(&fc_req->sg_req, rspn_req, sizeof(*rspn_req));
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated[1]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to remove strlcpy() completely[2], replace strlcpy() here with strscpy(). Overflow should be impossible here, but actually check for buffer sizes being identical with BUILD_BUG_ON(), and include a run-time check as well. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1] Link: https://github.com/KSPP/linux/issues/89 [2] Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> Cc: Steffen Maier <maier@linux.ibm.com> Cc: Benjamin Block <bblock@linux.ibm.com> Cc: Heiko Carstens <hca@linux.ibm.com> Cc: Vasily Gorbik <gor@linux.ibm.com> Cc: Alexander Gordeev <agordeev@linux.ibm.com> Cc: Christian Borntraeger <borntraeger@linux.ibm.com> Cc: Sven Schnelle <svens@linux.ibm.com> Cc: Azeem Shaikh <azeemshaikh38@gmail.com> Cc: linux-s390@vger.kernel.org Cc: linux-scsi@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- v2: - add BUILD_BUG_ON (bblock) - CC SCSI maintainers (bblock) v1: https://lore.kernel.org/all/20231116191435.work.581-kees@kernel.org/ --- drivers/s390/scsi/zfcp_fc.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)