On Thu, May 23, 2024 at 03:48:01PM -0700, Kees Cook wrote:
> It looks like this is another case of a non-terminated string being made
> terminated by strncpy into a string with 1 extra byte at the end:
>
> char label[EXT4_LABEL_MAX + 1];
> ...
> - memset(label, 0, sizeof(label));
> lock_buffer(sbi->s_sbh);
> - strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX);
> + strscpy_pad(label, sbi->s_es->s_volume_name);
> unlock_buffer(sbi->s_sbh);
>
> This should be using memtostr_pad() as:
>
> memtostr_pad(label, sbi->s_es->s_volume_name);
>
Ah... I see what is going on. The two argument variants of
memtostr_pad() and strscpy_pad() are confusing and dangerous. These
don't exist in the original OpenBSD strscpy() function, because the
size in the third argument is explicit, while with strscpy_pad(), the
automagic size is intuited from the first argument (the destination),
while with memtostr_pad(), the size is automagically intuited from the
second argument (the source).
This confused me, and I couldn't figure out the bug even when I was
given the stack trace from syzkaller. So it's an accident waiting to
happen, I clearly I was not smart enough not to fall into the trap,
So perhaps it might be nice if the descriptions of strscpy() is moved
out of process/deprecated.rst (and BTW, memstrtopad isn't mentioned at
all), and moved inta separate doumentation which safe string handling
in C --- so instead of talking about what functions *shouldn't* used,
such as strncpy(), it talks about how the various functions *should*
be used instead.
I'll also note that figuring out what was going on from looking at
include/linu/string.h was confusing, because there is so much #define
magic to provide the magic multiple argument handling. Personally, I
was never a fan of C++'s function overloading where different function
signatures could result in different implementations being called, and
doing with C preprocessor magic makes it even worse. To be fair,
there is the kernel-doc inline documentation, but my eyes were drawn
to the C++ implementation, and the kernel-doc documentation is more of
a reference and not a tutorial style "this is how you should do
things".
Anyway, thanks for sending the patch. I spent a good 30 minutes
trying to figure out the bug, and was half-tempted to just revert the
patch and go back to strncpy(), which at least I could obviously see
was correct, unlike the strscpy_pad() transmogrification.
> It looks like __nonstring markings from commit 072ebb3bffe6 ("ext4:
> add nonstring annotations to ext4.h") were incomplete.
Yes, I'll patch ext4.h to add a __nonstring annotation to
s_volume_name. As I recall, the reason why we had added the
__nonstring markings was to shut up gcc's -Wstringpop-truncation
warnings, and apparently it was needed for s_volume_name, which is why
it was never annotated.
Out of curiosity, though, would this have caused some analysis tool to
trigger a warning when the strscpy_pad() commit was added? I've
tried, and it doesn't seem to have triggered any kind of warning with
either gcc, clang, or sparse.
Anyway, since I'm an old fart, it was pretty obvious to *me* that the
how strcnpy() was used was obviouly correct, whereas I actually have
to do more careful thinking and analysis with strscpy() and
memtostr(). So it would be nice if there were some automated tools
that warn if those new functions aren't used correctly, because this
bug shows that these functions are definitely not fool proof --- both
by the original patch submitter, and the fool who reviewed the patch
and missed the problem. :-)
- Ted
