Hello Linus,
please pull a couple of s390 fixes for 6.5-rc3.
Thanks,
Heiko
The following changes since commit fdf0eaf11452d72945af31804e2a1048ee1b574c:
Linux 6.5-rc2 (2023-07-16 15:10:37 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.5-3
for you to fetch changes up to 4cfca532ddc3474b3fc42592d0e4237544344b1a:
s390/zcrypt: fix reply buffer calculations for CCA replies (2023-07-20 16:48:56 +0200)
----------------------------------------------------------------
s390 fixes for 6.5-rc3
- Fix per vma lock fault handling: add missing !(fault & VM_FAULT_ERROR)
check to fault handler to prevent error handling for return values that
don't indicate an error
- Use kfree_sensitive() instead of kfree() in paes crypto code to clear
memory that may contain keys before freeing it
- Fix reply buffer size calculation for CCA replies in zcrypt device driver
----------------------------------------------------------------
Harald Freudenberger (1):
s390/zcrypt: fix reply buffer calculations for CCA replies
Sven Schnelle (1):
s390/mm: fix per vma lock fault handling
Wang Ming (1):
s390/crypto: use kfree_sensitive() instead of kfree()
arch/s390/crypto/paes_s390.c | 2 +-
arch/s390/mm/fault.c | 2 ++
drivers/s390/crypto/zcrypt_msgtype6.c | 33 +++++++++++++++++++++++----------
3 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index d29a9d908797..38349150c96e 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -103,7 +103,7 @@ static inline void _free_kb_keybuf(struct key_blob *kb)
{
if (kb->key && kb->key != kb->keybuf
&& kb->keylen > sizeof(kb->keybuf)) {
- kfree(kb->key);
+ kfree_sensitive(kb->key);
kb->key = NULL;
}
}
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index dbe8394234e2..2f123429a291 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -421,6 +421,8 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access)
vma_end_read(vma);
if (!(fault & VM_FAULT_RETRY)) {
count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
+ if (likely(!(fault & VM_FAULT_ERROR)))
+ fault = 0;
goto out;
}
count_vm_vma_lock_event(VMA_LOCK_RETRY);
diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index 67fd2ec9c5a1..e668ff5eb384 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -1101,23 +1101,36 @@ static long zcrypt_msgtype6_send_cprb(bool userspace, struct zcrypt_queue *zq,
struct ica_xcRB *xcrb,
struct ap_message *ap_msg)
{
- int rc;
struct response_type *rtype = ap_msg->private;
struct {
struct type6_hdr hdr;
struct CPRBX cprbx;
/* ... more data blocks ... */
} __packed * msg = ap_msg->msg;
+ unsigned int max_payload_size;
+ int rc, delta;
- /*
- * Set the queue's reply buffer length minus 128 byte padding
- * as reply limit for the card firmware.
- */
- msg->hdr.fromcardlen1 = min_t(unsigned int, msg->hdr.fromcardlen1,
- zq->reply.bufsize - 128);
- if (msg->hdr.fromcardlen2)
- msg->hdr.fromcardlen2 =
- zq->reply.bufsize - msg->hdr.fromcardlen1 - 128;
+ /* calculate maximum payload for this card and msg type */
+ max_payload_size = zq->reply.bufsize - sizeof(struct type86_fmt2_msg);
+
+ /* limit each of the two from fields to the maximum payload size */
+ msg->hdr.fromcardlen1 = min(msg->hdr.fromcardlen1, max_payload_size);
+ msg->hdr.fromcardlen2 = min(msg->hdr.fromcardlen2, max_payload_size);
+
+ /* calculate delta if the sum of both exceeds max payload size */
+ delta = msg->hdr.fromcardlen1 + msg->hdr.fromcardlen2
+ - max_payload_size;
+ if (delta > 0) {
+ /*
+ * Sum exceeds maximum payload size, prune fromcardlen1
+ * (always trust fromcardlen2)
+ */
+ if (delta > msg->hdr.fromcardlen1) {
+ rc = -EINVAL;
+ goto out;
+ }
+ msg->hdr.fromcardlen1 -= delta;
+ }
init_completion(&rtype->work);
rc = ap_queue_message(zq->queue, ap_msg);
On Sat, 22 Jul 2023 at 09:02, Heiko Carstens <[email protected]> wrote:
>
> - Fix per vma lock fault handling: add missing !(fault & VM_FAULT_ERROR)
> check to fault handler to prevent error handling for return values that
> don't indicate an error
Hmm. The s390 code / people seems to still be a bit confused about the
VM_FAULT flags.
The commit comment says "With per-vma locks, handle_mm_fault() may
return non-fatal error flags".
That's actively misleading.
Why? Because handle_mm_fault() may - and will - return non-fatal error
flags *regardless* of the per-vma locks.
There's VM_FAULT_COMPLETED, there's VM_FAULT_MAJOR, there are all
those kinds of "informational" bits.
So honestly, when that patch then does
+ if (likely(!(fault & VM_FAULT_ERROR)))
+ fault = 0;
I feel like the code is very confused about what is going on, and is
papering over the real bug.
The *real* bug seems to be that do_protection_exception() and
do_dat_exception() do this:
fault = do_exception(regs, access);
if (unlikely(fault))
do_fault_error(regs, fault);
which is basically nonsensical. And the reason that s390 does that
seems to be that s390 (and arm, for that matter) seem, to have added a
few extra VM_FAULT_xyz bits that aren't part of VM_FAULT_ERROR, so
then in do_fault_error() you have that nonsensical "test some of the
fields as values, and other fields as bits".
Anyway, I have pulled this, since it clearly fixes a problem. But I do
think that the *deeper* problem is that s390 treats those bits as
errors in the first place, when they really aren't. Yes, the error
bits are *common*, but that field really shouldn't be seen as just
errors, and I really think that the deeper problem is that
if (unlikely(fault))
do_fault_error(regs, fault);
logic. It's simply wrong.
Of course, it looks like the reason you found this is that the s390
do_fault_error() then does a BUG() on any bits it doesn't understand.
You have that nonsensical "clear flags" in other places too. So it's
not like this work-around is new. But it's a workaround, and a sign of
confusion, I feel.
Maybe the extra s390 fault conditions should be added to the generic
list and added to the VM_FAULT_ERROR mask. I dunno.
Linus
The pull request you sent on Sat, 22 Jul 2023 18:02:15 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.5-3
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/295e1388de2d5c0c354adbd65d0319c5d636c222
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On Sat, Jul 22, 2023 at 11:52:22AM -0700, Linus Torvalds wrote:
> On Sat, 22 Jul 2023 at 09:02, Heiko Carstens <[email protected]> wrote:
> >
> > - Fix per vma lock fault handling: add missing !(fault & VM_FAULT_ERROR)
> > check to fault handler to prevent error handling for return values that
> > don't indicate an error
>
> Hmm. The s390 code / people seems to still be a bit confused about the
> VM_FAULT flags.
>
> The commit comment says "With per-vma locks, handle_mm_fault() may
> return non-fatal error flags".
>
> That's actively misleading.
...
> Anyway, I have pulled this, since it clearly fixes a problem. But I do
> think that the *deeper* problem is that s390 treats those bits as
> errors in the first place, when they really aren't. Yes, the error
> bits are *common*, but that field really shouldn't be seen as just
> errors, and I really think that the deeper problem is that
>
> if (unlikely(fault))
> do_fault_error(regs, fault);
>
> logic. It's simply wrong.
>
> Of course, it looks like the reason you found this is that the s390
> do_fault_error() then does a BUG() on any bits it doesn't understand.
> You have that nonsensical "clear flags" in other places too. So it's
> not like this work-around is new. But it's a workaround, and a sign of
> confusion, I feel.
>
> Maybe the extra s390 fault conditions should be added to the generic
> list and added to the VM_FAULT_ERROR mask. I dunno.
Thanks for looking a bit deeper into the code. Our "special" private
VM_FAULT flags came already to attention a couple of months ago [1]. Most
of the flags are historic - I'll try to get rid of all of them, since for
other architectures it is also possible without having private flags.
Our fault handling code needs some refactoring anyway. Hopefully the result
will be easier to maintain, and makes it a bit more difficult to add bugs
like I recently introduced with this per-vma lock architecture backend.
[1] https://lore.kernel.org/all/[email protected]/