linux-kernel - Re: [PATCH RFC] Introduce uniptr_t as a generic "universal" pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wmy4ciap.wl-tiwai@suse.de>
Date:   Wed, 09 Aug 2023 18:05:50 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christoph Hellwig <hch@....de>, Takashi Iwai <tiwai@...e.de>,
        linux-kernel@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC] Introduce uniptr_t as a generic "universal" pointer

On Wed, 09 Aug 2023 17:48:11 +0200,
Linus Torvalds wrote:
> 
> On Wed, 9 Aug 2023 at 07:38, Christoph Hellwig <hch@....de> wrote:
> >
> > The original set_fs removal series did that as uptr_t, and Linus
> > hated it with passion.  I somehow doubt he's going to like it more now.
> 
> Christoph is right. I do hate this. The whole "pass a pointer that is
> either user or kernel" concept is wrong.
> 
> Now, if it was some kind of extended pointer that also included the
> length of the area and had a way to deal with updating the pointer
> sanely, maybe that would be a different thing.
> 
> And it should guarantee that in the case of a user pointer it had gone
> through access_ok().
> 
> And it also allowed the other common cases like having a raw page
> array, along with a unified interface to copy and update this kind of
> pointer either as a source or a destination, that would be a different
> thing.
> 
> But this kind of "if (uniptr_is_kernel(src))" special case thing is
> just garbage and *not* acceptable.
> 
> And oh, btw, we already *have* that extended kind of unipointer thing.
> 
> It's called "struct iov_iter".
> 
> And yes, it's a very complicated thing, exactly because it handles way
> more cases than that uniptr_t. It's a *real* unified pointer of many
> different types.
> 
> Those iov_iter things are often so complicated that you really don't
> want to use them, but if you really want a uniptr, that is what you
> should do. It comes with a real cost, but it does come with real
> advantages, one of which is "this is extensively tested
> nfrastructure".

Hmm.  In one side, I tend to agree that it can go wrong easily.

OTOH, it simplifies the code well for us; as of now, we have two
callbacks for copying PCM memory from/to the device, distinct for
kernel and user pointers.  It's basically either copy_from_user() or
memcpy() of the given size depending on the caller.  The sockptr_t or
its variant would allow us to unify those to a single callback.

Of course, we can create yet another local type that is just for the
specific code -- or just (ab)use sockptr_t as is.  The fact is that it
can simplify the code well, which in turn make more maintainable.

Though, I have no strong opinion about this topic.  If you believe
this kind of code is way too dangerous, fine, we can go with the
current code.  OTOH, if the limited use is acceptable (as already seen
with sockptr_t), we can either re-use it or have own one.

(And yeah, iov_iter is there, but it's definitely overkill for the
purpose.)


Takashi