Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 10 Jul 2019 18:54:40 -0700
From:   Kees Cook <keescook@chromium.org>
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/16] nfsd: escape high characters in binary data
Message-ID: <201907101853.AB9F346A92@keescook>
References: <20190621174544.GC25590@fieldses.org>
 <201906211431.E6552108@keescook>
 <20190622190058.GD5343@fieldses.org>
 <201906221320.5BFC134713@keescook>
 <20190624210512.GA20331@fieldses.org>
 <20190626162149.GB4144@fieldses.org>
 <20190627202124.GC16388@fieldses.org>
 <201906272054.6954C08FA@keescook>
 <20190628163358.GA31800@fieldses.org>
 <20190710220931.GB11923@fieldses.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190710220931.GB11923@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Wed, Jul 10, 2019 at 06:09:31PM -0400, J. Bruce Fields wrote:
> On Fri, Jun 28, 2019 at 12:33:58PM -0400, J. Bruce Fields wrote:
> > But I may just give up and go with my existing patch and put
> > off that project indefinitely, especially if there's no real need to fix
> > the existing callers.
> 
> I went with the existing patch, but gave a little more thought to
> string_escape_mem.  Stuff that bugs me:
> 
> 	- ESCAPE_NP sounds like it means "escape nonprinting
> 	  characters", but actually means "do not escape printing
> 	  characters"
> 	- the use of the "only" string to limit the list of escaped
> 	  characters rather than supplement them is confusing and kind
> 	  of unhelpful.
> 	- most of the flags are actually totally unused
>     
> So what I'd like to do is:
>     
> 	- eliminate unused flags
> 	- use the "only" string to add to, rather than replace, the list
> 	  of characters to escape
> 	- separate flags into those that select which characters to
> 	  escape, and those that choose the format of the escaping ("\ "
> 	  vs "\x20" vs "\040".)
>     
> I've got some patches that do all that and I think it works.  I need to
> clean them up a bit and fix up the tests.

This sounds amazing; thanks! Luckily there are self-tests for this code,
so anything really surprising should stand out. I'm looking forward to
it -- I want to see if I can refactor a few of the callers (if you
haven't already do so) too.

Yay!

-- 
Kees Cook