Re: stack smashing detected with 'nvme sanitize-log /dev/nvme0'

[Keith Busch <kbusch@kernel.org>]

On Tue, Aug 22, 2023 at 09:55:38AM +0200, Daniel Wagner wrote:
> On Mon, Aug 21, 2023 at 09:11:38AM -0600, Keith Busch wrote:
> > I don't think we want to bounce to kernel memory for the device to
> > overwrite it. I suggest just change nvme-cli's stack allocated santize
> > log to a use page aligned and sized buffer.
> 
> Sure we can add workarounds in userspace, though I think this is a
> regression and should ideally address in the kernel somehow.

Not sure if this is a regression. Even prior to the DMA alignment
settings change, you'd still hit this if your stack buffer aligned to
512b. The kernel bounce buffer sounds like it just corrupts kernel
memory instead, so we can't have the driver just go back to the way it
was for these devices; someone has to over allocate the buffer.

A device over running their DMA buffer, though, sounds bad enough that a
firmware fix should happen instead.

> > diff --git a/nvme.c b/nvme.c
> > index 181141ad..7f98a506 100644
> > --- a/nvme.c
> > +++ b/nvme.c
> > @@ -2376,7 +2376,7 @@ ret:
> >  static int sanitize_log(int argc, char **argv, struct command *command, struct plugin *plugin)
> >  {
> >  	const char *desc = "Retrieve sanitize log and show it.";
> > -	struct nvme_sanitize_log_page sanitize_log;
> > +	struct nvme_sanitize_log_page *sanitize_log;
> >  	enum nvme_print_flags flags;
> >  	struct nvme_dev *dev;
> >  	int err;
> > @@ -2419,13 +2419,19 @@ static int sanitize_log(int argc, char **argv, struct command *command, struct p
> >  	if (cfg.human_readable)
> >  		flags |= VERBOSE;
> >
> > -	err = nvme_cli_get_log_sanitize(dev, cfg.rae, &sanitize_log);
> > +	if (posix_memalign((void *)&sanitize_log, getpagesize(), 0x1000)) {
> > +		err = -1;
> > +		goto close_dev;
> > +	}
> > +
> > +	err = nvme_cli_get_log_sanitize(dev, cfg.rae, sanitize_log);
> >  	if (!err)
> > -		nvme_show_sanitize_log(&sanitize_log, dev->name, flags);
> > +		nvme_show_sanitize_log(sanitize_log, dev->name, flags);
> >  	else if (err > 0)
> >  		nvme_show_status(err);
> >  	else
> >  		nvme_show_error("sanitize status log: %s", nvme_strerror(errno));
> > +	free(sanitize_log);
> >  close_dev:
> >  	dev_close(dev);
> >  ret:
> 
> Anyway, I suppose we want to do this far all get log commands, not sure
> if it is limited to the get sanitize log alone.

Perhaps for saftey we could do that for all logs < 4k in size. There are
not very many of those.