* [PATCH] s390/zcrypt: Use scnprintf() for avoiding potential buffer overflow
@ 2020-03-11 9:09 Takashi Iwai
2020-03-12 10:17 ` Harald Freudenberger
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2020-03-11 9:09 UTC (permalink / raw)
To: Harald Freudenberger
Cc: Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390
Since snprintf() returns the would-be-output size instead of the
actual output size, the succeeding calls may go beyond the given
buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/s390/crypto/zcrypt_cex4.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/s390/crypto/zcrypt_cex4.c b/drivers/s390/crypto/zcrypt_cex4.c
index 9a9d02e19774..dea96d5b65fb 100644
--- a/drivers/s390/crypto/zcrypt_cex4.c
+++ b/drivers/s390/crypto/zcrypt_cex4.c
@@ -128,16 +128,16 @@ static ssize_t cca_mkvps_show(struct device *dev,
n = snprintf(buf, PAGE_SIZE, "AES NEW: - -\n");
if (ci.cur_mk_state >= '1' && ci.cur_mk_state <= '2')
- n += snprintf(buf + n, PAGE_SIZE - n, "AES CUR: %s 0x%016llx\n",
+ n += scnprintf(buf + n, PAGE_SIZE - n, "AES CUR: %s 0x%016llx\n",
cao_state[ci.cur_mk_state - '1'], ci.cur_mkvp);
else
- n += snprintf(buf + n, PAGE_SIZE - n, "AES CUR: - -\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "AES CUR: - -\n");
if (ci.old_mk_state >= '1' && ci.old_mk_state <= '2')
- n += snprintf(buf + n, PAGE_SIZE - n, "AES OLD: %s 0x%016llx\n",
+ n += scnprintf(buf + n, PAGE_SIZE - n, "AES OLD: %s 0x%016llx\n",
cao_state[ci.old_mk_state - '1'], ci.old_mkvp);
else
- n += snprintf(buf + n, PAGE_SIZE - n, "AES OLD: - -\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "AES OLD: - -\n");
return n;
}
@@ -251,11 +251,11 @@ static ssize_t ep11_card_op_modes_show(struct device *dev,
if (ci.op_mode & (1 << ep11_op_modes[i].mode_bit)) {
if (n > 0)
buf[n++] = ' ';
- n += snprintf(buf + n, PAGE_SIZE - n,
+ n += scnprintf(buf + n, PAGE_SIZE - n,
"%s", ep11_op_modes[i].mode_txt);
}
}
- n += snprintf(buf + n, PAGE_SIZE - n, "\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
return n;
}
@@ -305,21 +305,21 @@ static ssize_t ep11_mkvps_show(struct device *dev,
cwk_state[di.cur_wk_state - '0']);
bin2hex(buf + n, di.cur_wkvp, sizeof(di.cur_wkvp));
n += 2 * sizeof(di.cur_wkvp);
- n += snprintf(buf + n, PAGE_SIZE - n, "\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
} else
n = snprintf(buf, PAGE_SIZE, "WK CUR: - -\n");
if (di.new_wk_state == '0') {
- n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s -\n",
+ n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s -\n",
nwk_state[di.new_wk_state - '0']);
} else if (di.new_wk_state >= '1' && di.new_wk_state <= '2') {
- n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s 0x",
+ n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s 0x",
nwk_state[di.new_wk_state - '0']);
bin2hex(buf + n, di.new_wkvp, sizeof(di.new_wkvp));
n += 2 * sizeof(di.new_wkvp);
- n += snprintf(buf + n, PAGE_SIZE - n, "\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
} else
- n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: - -\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: - -\n");
return n;
}
@@ -346,11 +346,11 @@ static ssize_t ep11_queue_op_modes_show(struct device *dev,
if (di.op_mode & (1 << ep11_op_modes[i].mode_bit)) {
if (n > 0)
buf[n++] = ' ';
- n += snprintf(buf + n, PAGE_SIZE - n,
+ n += scnprintf(buf + n, PAGE_SIZE - n,
"%s", ep11_op_modes[i].mode_txt);
}
}
- n += snprintf(buf + n, PAGE_SIZE - n, "\n");
+ n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
return n;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] s390/zcrypt: Use scnprintf() for avoiding potential buffer overflow
2020-03-11 9:09 [PATCH] s390/zcrypt: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai
@ 2020-03-12 10:17 ` Harald Freudenberger
2020-03-12 11:39 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Harald Freudenberger @ 2020-03-12 10:17 UTC (permalink / raw)
To: Takashi Iwai
Cc: Heiko Carstens, Vasily Gorbik, Christian Borntraeger, linux-s390
On 11.03.20 10:09, Takashi Iwai wrote:
> Since snprintf() returns the would-be-output size instead of the
> actual output size, the succeeding calls may go beyond the given
> buffer limit. Fix it by replacing with scnprintf().
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> drivers/s390/crypto/zcrypt_cex4.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/s390/crypto/zcrypt_cex4.c b/drivers/s390/crypto/zcrypt_cex4.c
> index 9a9d02e19774..dea96d5b65fb 100644
> --- a/drivers/s390/crypto/zcrypt_cex4.c
> +++ b/drivers/s390/crypto/zcrypt_cex4.c
> @@ -128,16 +128,16 @@ static ssize_t cca_mkvps_show(struct device *dev,
> n = snprintf(buf, PAGE_SIZE, "AES NEW: - -\n");
>
> if (ci.cur_mk_state >= '1' && ci.cur_mk_state <= '2')
> - n += snprintf(buf + n, PAGE_SIZE - n, "AES CUR: %s 0x%016llx\n",
> + n += scnprintf(buf + n, PAGE_SIZE - n, "AES CUR: %s 0x%016llx\n",
> cao_state[ci.cur_mk_state - '1'], ci.cur_mkvp);
> else
> - n += snprintf(buf + n, PAGE_SIZE - n, "AES CUR: - -\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "AES CUR: - -\n");
>
> if (ci.old_mk_state >= '1' && ci.old_mk_state <= '2')
> - n += snprintf(buf + n, PAGE_SIZE - n, "AES OLD: %s 0x%016llx\n",
> + n += scnprintf(buf + n, PAGE_SIZE - n, "AES OLD: %s 0x%016llx\n",
> cao_state[ci.old_mk_state - '1'], ci.old_mkvp);
> else
> - n += snprintf(buf + n, PAGE_SIZE - n, "AES OLD: - -\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "AES OLD: - -\n");
>
> return n;
> }
> @@ -251,11 +251,11 @@ static ssize_t ep11_card_op_modes_show(struct device *dev,
> if (ci.op_mode & (1 << ep11_op_modes[i].mode_bit)) {
> if (n > 0)
> buf[n++] = ' ';
> - n += snprintf(buf + n, PAGE_SIZE - n,
> + n += scnprintf(buf + n, PAGE_SIZE - n,
> "%s", ep11_op_modes[i].mode_txt);
> }
> }
> - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
>
> return n;
> }
> @@ -305,21 +305,21 @@ static ssize_t ep11_mkvps_show(struct device *dev,
> cwk_state[di.cur_wk_state - '0']);
> bin2hex(buf + n, di.cur_wkvp, sizeof(di.cur_wkvp));
> n += 2 * sizeof(di.cur_wkvp);
> - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
> } else
> n = snprintf(buf, PAGE_SIZE, "WK CUR: - -\n");
>
> if (di.new_wk_state == '0') {
> - n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s -\n",
> + n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s -\n",
> nwk_state[di.new_wk_state - '0']);
> } else if (di.new_wk_state >= '1' && di.new_wk_state <= '2') {
> - n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s 0x",
> + n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s 0x",
> nwk_state[di.new_wk_state - '0']);
> bin2hex(buf + n, di.new_wkvp, sizeof(di.new_wkvp));
> n += 2 * sizeof(di.new_wkvp);
> - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
> } else
> - n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: - -\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: - -\n");
>
> return n;
> }
> @@ -346,11 +346,11 @@ static ssize_t ep11_queue_op_modes_show(struct device *dev,
> if (di.op_mode & (1 << ep11_op_modes[i].mode_bit)) {
> if (n > 0)
> buf[n++] = ' ';
> - n += snprintf(buf + n, PAGE_SIZE - n,
> + n += scnprintf(buf + n, PAGE_SIZE - n,
> "%s", ep11_op_modes[i].mode_txt);
> }
> }
> - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
>
> return n;
> }
Thanks for this patch.
I will use this as a trigger point to rework all the snprintfs within the ap bus and zcrypt code
and replace them with scnprintf wherever the return code is used. Did not know that
there are issues with snprintf ... This article gives some background: https://lwn.net/Articles/69419/
The updates will go into the s390 subsystem and will be forwarded to the upstream kernel with the
next kernel merge window.
Harald Freudenberger
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] s390/zcrypt: Use scnprintf() for avoiding potential buffer overflow
2020-03-12 10:17 ` Harald Freudenberger
@ 2020-03-12 11:39 ` Takashi Iwai
0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2020-03-12 11:39 UTC (permalink / raw)
To: Harald Freudenberger
Cc: Takashi Iwai, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, linux-s390
On Thu, 12 Mar 2020 11:17:14 +0100,
Harald Freudenberger wrote:
>
> On 11.03.20 10:09, Takashi Iwai wrote:
> > Since snprintf() returns the would-be-output size instead of the
> > actual output size, the succeeding calls may go beyond the given
> > buffer limit. Fix it by replacing with scnprintf().
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > drivers/s390/crypto/zcrypt_cex4.c | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/s390/crypto/zcrypt_cex4.c b/drivers/s390/crypto/zcrypt_cex4.c
> > index 9a9d02e19774..dea96d5b65fb 100644
> > --- a/drivers/s390/crypto/zcrypt_cex4.c
> > +++ b/drivers/s390/crypto/zcrypt_cex4.c
> > @@ -128,16 +128,16 @@ static ssize_t cca_mkvps_show(struct device *dev,
> > n = snprintf(buf, PAGE_SIZE, "AES NEW: - -\n");
> >
> > if (ci.cur_mk_state >= '1' && ci.cur_mk_state <= '2')
> > - n += snprintf(buf + n, PAGE_SIZE - n, "AES CUR: %s 0x%016llx\n",
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "AES CUR: %s 0x%016llx\n",
> > cao_state[ci.cur_mk_state - '1'], ci.cur_mkvp);
> > else
> > - n += snprintf(buf + n, PAGE_SIZE - n, "AES CUR: - -\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "AES CUR: - -\n");
> >
> > if (ci.old_mk_state >= '1' && ci.old_mk_state <= '2')
> > - n += snprintf(buf + n, PAGE_SIZE - n, "AES OLD: %s 0x%016llx\n",
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "AES OLD: %s 0x%016llx\n",
> > cao_state[ci.old_mk_state - '1'], ci.old_mkvp);
> > else
> > - n += snprintf(buf + n, PAGE_SIZE - n, "AES OLD: - -\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "AES OLD: - -\n");
> >
> > return n;
> > }
> > @@ -251,11 +251,11 @@ static ssize_t ep11_card_op_modes_show(struct device *dev,
> > if (ci.op_mode & (1 << ep11_op_modes[i].mode_bit)) {
> > if (n > 0)
> > buf[n++] = ' ';
> > - n += snprintf(buf + n, PAGE_SIZE - n,
> > + n += scnprintf(buf + n, PAGE_SIZE - n,
> > "%s", ep11_op_modes[i].mode_txt);
> > }
> > }
> > - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
> >
> > return n;
> > }
> > @@ -305,21 +305,21 @@ static ssize_t ep11_mkvps_show(struct device *dev,
> > cwk_state[di.cur_wk_state - '0']);
> > bin2hex(buf + n, di.cur_wkvp, sizeof(di.cur_wkvp));
> > n += 2 * sizeof(di.cur_wkvp);
> > - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
> > } else
> > n = snprintf(buf, PAGE_SIZE, "WK CUR: - -\n");
> >
> > if (di.new_wk_state == '0') {
> > - n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s -\n",
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s -\n",
> > nwk_state[di.new_wk_state - '0']);
> > } else if (di.new_wk_state >= '1' && di.new_wk_state <= '2') {
> > - n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s 0x",
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: %s 0x",
> > nwk_state[di.new_wk_state - '0']);
> > bin2hex(buf + n, di.new_wkvp, sizeof(di.new_wkvp));
> > n += 2 * sizeof(di.new_wkvp);
> > - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
> > } else
> > - n += snprintf(buf + n, PAGE_SIZE - n, "WK NEW: - -\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "WK NEW: - -\n");
> >
> > return n;
> > }
> > @@ -346,11 +346,11 @@ static ssize_t ep11_queue_op_modes_show(struct device *dev,
> > if (di.op_mode & (1 << ep11_op_modes[i].mode_bit)) {
> > if (n > 0)
> > buf[n++] = ' ';
> > - n += snprintf(buf + n, PAGE_SIZE - n,
> > + n += scnprintf(buf + n, PAGE_SIZE - n,
> > "%s", ep11_op_modes[i].mode_txt);
> > }
> > }
> > - n += snprintf(buf + n, PAGE_SIZE - n, "\n");
> > + n += scnprintf(buf + n, PAGE_SIZE - n, "\n");
> >
> > return n;
> > }
>
> Thanks for this patch.
>
> I will use this as a trigger point to rework all the snprintfs within the ap bus and zcrypt code
> and replace them with scnprintf wherever the return code is used. Did not know that
> there are issues with snprintf ... This article gives some background: https://lwn.net/Articles/69419/
>
> The updates will go into the s390 subsystem and will be forwarded to the upstream kernel with the
> next kernel merge window.
Covering the whole snprintf() usage would be nicer, yes.
Thanks!
Takashi
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-12 11:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 9:09 [PATCH] s390/zcrypt: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai
2020-03-12 10:17 ` Harald Freudenberger
2020-03-12 11:39 ` Takashi Iwai
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.