All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <david.laight.linux@gmail.com>
To: op-tee@lists.trustedfirmware.org
Subject: Re: [PATCH] tee: Prevent size calculation wraparound on 32-bit kernels
Date: Fri, 02 May 2025 12:41:01 +0000	[thread overview]
Message-ID: <20250502134058.0f3e79cc@pumpkin> (raw)
In-Reply-To: < <CAG48ez1DPWhT2dhd1iptFawWjteh_=pZ4M6Yq5KKCq2DTArnqw@mail.gmail.com>>

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

On Fri, 2 May 2025 14:28:21 +0200
Jann Horn <jannh@google.com> wrote:

> On Thu, May 1, 2025 at 10:02 PM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Mon, 28 Apr 2025 15:06:43 +0200
> > Jann Horn <jannh@google.com> wrote:
> >  
> > > The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on
> > > 32-bit kernels: Multiplying a user-provided 32-bit value with the
> > > size of a structure can wrap around on such platforms.
> > >
> > > Fix it by using saturating arithmetic for the size calculation.  
> >
> > Why not just add a sanity check on 'num_params' after it is read.
> > Max is 31 (1024-32)/32), but any sane limit will do because of
> > the buf.buf_len test.  
> 
> That would work, too. I don't know which way looks nicer.

The saturating arithmetic functions are non-obvious and non-trivial.
I looked at the code to check where buf.buf_len came from,
without its sanity check the user could craft a request where it
matched the saturated size.

I think I'd sanity check the number of items and then check that that
buffer length is right for the number of items.

	David

WARNING: multiple messages have this Message-ID (diff)
From: David Laight <david.laight.linux@gmail.com>
To: Jann Horn <jannh@google.com>
Cc: Jens Wiklander <jens.wiklander@linaro.org>,
	Sumit Garg <sumit.garg@kernel.org>,
	op-tee@lists.trustedfirmware.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tee: Prevent size calculation wraparound on 32-bit kernels
Date: Fri, 2 May 2025 13:40:58 +0100	[thread overview]
Message-ID: <20250502134058.0f3e79cc@pumpkin> (raw)
In-Reply-To: <CAG48ez1DPWhT2dhd1iptFawWjteh_=pZ4M6Yq5KKCq2DTArnqw@mail.gmail.com>

On Fri, 2 May 2025 14:28:21 +0200
Jann Horn <jannh@google.com> wrote:

> On Thu, May 1, 2025 at 10:02 PM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Mon, 28 Apr 2025 15:06:43 +0200
> > Jann Horn <jannh@google.com> wrote:
> >  
> > > The current code around TEE_IOCTL_PARAM_SIZE() is a bit wrong on
> > > 32-bit kernels: Multiplying a user-provided 32-bit value with the
> > > size of a structure can wrap around on such platforms.
> > >
> > > Fix it by using saturating arithmetic for the size calculation.  
> >
> > Why not just add a sanity check on 'num_params' after it is read.
> > Max is 31 (1024-32)/32), but any sane limit will do because of
> > the buf.buf_len test.  
> 
> That would work, too. I don't know which way looks nicer.

The saturating arithmetic functions are non-obvious and non-trivial.
I looked at the code to check where buf.buf_len came from,
without its sanity check the user could craft a request where it
matched the saturated size.

I think I'd sanity check the number of items and then check that that
buffer length is right for the number of items.

	David

  parent reply	other threads:[~2025-05-02 12:41 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-28 13:06 [PATCH] tee: Prevent size calculation wraparound on 32-bit kernels Jann Horn
2025-04-28 13:06 ` Jann Horn
2025-04-30 11:52 ` Jens Wiklander
2025-04-30 11:52   ` Jens Wiklander
2025-05-01 20:01 ` David Laight
2025-05-01 20:01   ` David Laight
2025-05-02 12:28   ` Jann Horn
2025-05-02 12:28     ` Jann Horn
2025-05-02 12:40     ` David Laight [this message]
2025-05-02 12:41       ` David Laight
     [not found] < <CAHUa44E_JZdYnGrReP0zWCP1wdu2BdJ9DSZZ3a2OiobRj61ThQ@mail.gmail.com>
2025-04-30 12:12 ` Rouven Czerwinski
2025-04-30 12:12   ` Rouven Czerwinski
     [not found] < <CAK8z29XWCujUdLXcHz075+xcix8HV2Mp3EtxxX9GB7vGjwi3HA@mail.gmail.com>
2025-04-30 13:00 ` Jens Wiklander
2025-04-30 13:00   ` Jens Wiklander

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250502134058.0f3e79cc@pumpkin \
    --to=david.laight.linux@gmail.com \
    --cc=op-tee@lists.trustedfirmware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.