* [PATCH] libmultipath: fix ontap prioritizer snprintf limits @ 2024-08-02 17:26 Benjamin Marzinski 2024-08-06 19:12 ` Martin Wilck 0 siblings, 1 reply; 3+ messages in thread From: Benjamin Marzinski @ 2024-08-02 17:26 UTC (permalink / raw) To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck The ontap prioritizer functions dump_cdb() and process_sg_error() both incorrectly set the snprintf() limits larger than the available space. Instead of multiplying the number of elements to print by the size of an element to calculate the limit, they multiplied the number of elements to print by the maximum number of elements that the buffer could hold. Fix this, and also make sure that the number of elements to print is less than or equal to the maximum number that the buffer can hold. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/prioritizers/ontap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libmultipath/prioritizers/ontap.c b/libmultipath/prioritizers/ontap.c index 117886ea..28e663ac 100644 --- a/libmultipath/prioritizers/ontap.c +++ b/libmultipath/prioritizers/ontap.c @@ -39,8 +39,8 @@ static void dump_cdb(unsigned char *cdb, int size) char * p = &buf[0]; condlog(0, "- SCSI CDB: "); - for (i=0; i<size; i++) { - p += snprintf(p, 10*(size-i), "0x%02x ", cdb[i]); + for (i = 0; i < size && i < 10; i++) { + p += snprintf(p, 5*(size-i), "0x%02x ", cdb[i]); } condlog(0, "%s", buf); } @@ -56,8 +56,8 @@ static void process_sg_error(struct sg_io_hdr *io_hdr) io_hdr->host_status, io_hdr->driver_status); if (io_hdr->sb_len_wr > 0) { condlog(0, "- SCSI sense data: "); - for (i=0; i<io_hdr->sb_len_wr; i++) { - p += snprintf(p, 128*(io_hdr->sb_len_wr-i), "0x%02x ", + for (i = 0; i < io_hdr->sb_len_wr && i < 128; i++) { + p += snprintf(p, 5*(io_hdr->sb_len_wr-i), "0x%02x ", io_hdr->sbp[i]); } condlog(0, "%s", buf); -- 2.45.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libmultipath: fix ontap prioritizer snprintf limits 2024-08-02 17:26 [PATCH] libmultipath: fix ontap prioritizer snprintf limits Benjamin Marzinski @ 2024-08-06 19:12 ` Martin Wilck 2024-08-07 20:35 ` Benjamin Marzinski 0 siblings, 1 reply; 3+ messages in thread From: Martin Wilck @ 2024-08-06 19:12 UTC (permalink / raw) To: Benjamin Marzinski, Christophe Varoqui; +Cc: device-mapper development On Fri, 2024-08-02 at 13:26 -0400, Benjamin Marzinski wrote: > The ontap prioritizer functions dump_cdb() and process_sg_error() > both > incorrectly set the snprintf() limits larger than the available > space. > Instead of multiplying the number of elements to print by the size of > an > element to calculate the limit, they multiplied the number of > elements > to print by the maximum number of elements that the buffer could > hold. > > Fix this, and also make sure that the number of elements to print is > less than or equal to the maximum number that the buffer can hold. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/prioritizers/ontap.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/libmultipath/prioritizers/ontap.c > b/libmultipath/prioritizers/ontap.c > index 117886ea..28e663ac 100644 > --- a/libmultipath/prioritizers/ontap.c > +++ b/libmultipath/prioritizers/ontap.c > @@ -39,8 +39,8 @@ static void dump_cdb(unsigned char *cdb, int size) > char * p = &buf[0]; > > condlog(0, "- SCSI CDB: "); > - for (i=0; i<size; i++) { > - p += snprintf(p, 10*(size-i), "0x%02x ", cdb[i]); > + for (i = 0; i < size && i < 10; i++) { > + p += snprintf(p, 5*(size-i), "0x%02x ", cdb[i]); > } This seems incorrect, too. We should pass the remaining size of the buffer, which is ((10 - i) * 5 + 1), unless I am mistaken. It's bogus to use the "size" variable in the calculation of the remaining buffer size. Plus, we should break the loop if i >= 10, and check the return value of snprintf (if we don't, we might as well use sprintf in the first place). Actually, this code is horrible and we should rather use strbuf. Strange that none of our many compilers found this, and even coverity didn't. > condlog(0, "%s", buf); > } > @@ -56,8 +56,8 @@ static void process_sg_error(struct sg_io_hdr > *io_hdr) > io_hdr->host_status, io_hdr->driver_status); > if (io_hdr->sb_len_wr > 0) { > condlog(0, "- SCSI sense data: "); > - for (i=0; i<io_hdr->sb_len_wr; i++) { > - p += snprintf(p, 128*(io_hdr->sb_len_wr-i), > "0x%02x ", > + for (i = 0; i < io_hdr->sb_len_wr && i < 128; i++) { > + p += snprintf(p, 5*(io_hdr->sb_len_wr-i), > "0x%02x ", > io_hdr->sbp[i]); > } Same here, the remaining buffer size is (5 * (128 - i) + 1). Martin > condlog(0, "%s", buf); ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libmultipath: fix ontap prioritizer snprintf limits 2024-08-06 19:12 ` Martin Wilck @ 2024-08-07 20:35 ` Benjamin Marzinski 0 siblings, 0 replies; 3+ messages in thread From: Benjamin Marzinski @ 2024-08-07 20:35 UTC (permalink / raw) To: Martin Wilck; +Cc: Christophe Varoqui, device-mapper development On Tue, Aug 06, 2024 at 09:12:21PM +0200, Martin Wilck wrote: > On Fri, 2024-08-02 at 13:26 -0400, Benjamin Marzinski wrote: > > The ontap prioritizer functions dump_cdb() and process_sg_error() > > both > > incorrectly set the snprintf() limits larger than the available > > space. > > Instead of multiplying the number of elements to print by the size of > > an > > element to calculate the limit, they multiplied the number of > > elements > > to print by the maximum number of elements that the buffer could > > hold. > > > > Fix this, and also make sure that the number of elements to print is > > less than or equal to the maximum number that the buffer can hold. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/prioritizers/ontap.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/libmultipath/prioritizers/ontap.c > > b/libmultipath/prioritizers/ontap.c > > index 117886ea..28e663ac 100644 > > --- a/libmultipath/prioritizers/ontap.c > > +++ b/libmultipath/prioritizers/ontap.c > > @@ -39,8 +39,8 @@ static void dump_cdb(unsigned char *cdb, int size) > > char * p = &buf[0]; > > > > condlog(0, "- SCSI CDB: "); > > - for (i=0; i<size; i++) { > > - p += snprintf(p, 10*(size-i), "0x%02x ", cdb[i]); > > + for (i = 0; i < size && i < 10; i++) { > > + p += snprintf(p, 5*(size-i), "0x%02x ", cdb[i]); > > } > > This seems incorrect, too. We should pass the remaining size of the > buffer, which is ((10 - i) * 5 + 1), unless I am mistaken. It's bogus > to use the "size" variable in the calculation of the remaining buffer > size. Plus, we should break the loop if i >= 10, and check the return > value of snprintf (if we don't, we might as well use sprintf in the > first place). > > Actually, this code is horrible and we should rather use strbuf. > Strange that none of our many compilers found this, and even coverity > didn't. > > > condlog(0, "%s", buf); > > } > > @@ -56,8 +56,8 @@ static void process_sg_error(struct sg_io_hdr > > *io_hdr) > > io_hdr->host_status, io_hdr->driver_status); > > if (io_hdr->sb_len_wr > 0) { > > condlog(0, "- SCSI sense data: "); > > - for (i=0; i<io_hdr->sb_len_wr; i++) { > > - p += snprintf(p, 128*(io_hdr->sb_len_wr-i), > > "0x%02x ", > > + for (i = 0; i < io_hdr->sb_len_wr && i < 128; i++) { > > + p += snprintf(p, 5*(io_hdr->sb_len_wr-i), > > "0x%02x ", > > io_hdr->sbp[i]); > > } > > Same here, the remaining buffer size is (5 * (128 - i) + 1). Sure. I'll send a v2. -Ben > Martin > > > condlog(0, "%s", buf); ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-07 20:35 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-02 17:26 [PATCH] libmultipath: fix ontap prioritizer snprintf limits Benjamin Marzinski 2024-08-06 19:12 ` Martin Wilck 2024-08-07 20:35 ` Benjamin Marzinski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.