On Fri, Feb 14, 2025 at 08:26:17AM -0700, Keith Busch wrote:
> On Fri, Feb 14, 2025 at 11:30:11AM +0800, Ming Lei wrote:
> > On Mon, Feb 10, 2025 at 04:56:43PM -0800, Keith Busch wrote:
> > > +
> > > + node->release = release;
> > > + node->priv = rq;
> > > +
> > > + nr_bvecs = blk_rq_nr_phys_segments(rq);
> > > + imu = kvmalloc(struct_size(imu, bvec, nr_bvecs), GFP_KERNEL);
> > > + if (!imu) {
> > > + kfree(node);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + imu->ubuf = 0;
> > > + imu->len = blk_rq_bytes(rq);
> > > + imu->acct_pages = 0;
> > > + imu->nr_bvecs = nr_bvecs;
> > > + refcount_set(&imu->refs, 1);
> > > + node->buf = imu;
> >
> > request buffer direction needs to be stored in `imu`, for READ,
> > the buffer is write-only, and for WRITE, the buffer is read-only,
> > which isn't different with user mapped buffer.
> >
> > Meantime in read_fixed/write_fixed side or buffer lookup abstraction
> > helper, the buffer direction needs to be validated.
>
> I suppose we could add that check, but the primary use case doesn't even
> use those operations. They're using uring_cmd with the FIXED flag, and
> io_uring can't readily validate the data direction from that interface.
The check can be added to io_import_fixed().
It is a security trouble. Without the validation:
- kernel data can be redirected to user file via write_fixed,
- kernel page data is over-written unexpectedly via read_fixed, cause fs corruption or
even kernel panic.
Thanks,
Ming
