linux-kernel - Re: [PATCH 1/1] s390/dasd: fix string length handling

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: <20230829080202.7031-B-hca@linux.ibm.com>
Date:   Tue, 29 Aug 2023 10:02:02 +0200
From:   Heiko Carstens <hca@...ux.ibm.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Jens Axboe <axboe@...nel.dk>, Stefan Haberland <sth@...ux.ibm.com>,
        Jan Höppner <hoeppner@...ux.ibm.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-block@...r.kernel.org, nathan@...nel.org,
        llvm@...ts.linux.dev, David Laight <David.Laight@...lab.com>
Subject: Re: [PATCH 1/1] s390/dasd: fix string length handling

On Mon, Aug 28, 2023 at 03:46:52PM -0700, Nick Desaulniers wrote:
> On Mon, Aug 28, 2023 at 05:31:42PM +0200, Heiko Carstens wrote:
> > Building dasd_eckd.o with latest clang reveals this bug:
> > 
> >     CC      drivers/s390/block/dasd_eckd.o
> >       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
> >       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
> >        1082 |                 snprintf(print_uid, sizeof(*print_uid),
> >             |                 ^
> >       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
> >       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
> >        1087 |                 snprintf(print_uid, sizeof(*print_uid),
> >             |                 ^
> > 
> > Fix this by moving and using the existing UID_STRLEN for the arrays
> > that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> > to clarify its scope.
> > 
> > Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
> > Reviewed-by: Peter Oberparleiter <oberpar@...ux.ibm.com>
> > Signed-off-by: Heiko Carstens <hca@...ux.ibm.com>
> 
> Thanks for the patch! Nathan just reported a bunch of these. I took a
> look at these two and thought "yeah that's clearly a bug in the kernel
> sources." Fix LGTM.
> 
> Reported-by: Nathan Chancellor <nathan@...nel.org>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
> Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
> 
> I also like David's idea of passing `char ident [DASD_UID_STRLEN]`, too,
> but I don't feel strongly either way.

Well, this is supposed to be the "minimal" fix. I consider everything else
additional cleanup work, which can and should be done by Stefan and Jan who
maintain this device driver.

For example there is more or less identical code within dasd_devmap.c
(dasd_uid_show()), where it would make sense to de-deduplicate the
code. And then of course there is the already mentioned rather pointless
strlen() invocation; plus there are many other string operations / format
strings, which also should be addressed.
E.g. there are quite a couple of "%p" printk format specifiers which are
pointless, since pointer values get hashed since years - so a more or less
random value will be printed, etc.

However all of this is up to Stefan and Jan.

So I consider this current fix as good enough and final.