On 1/26/24 14:35, Nathan Chancellor wrote:
(slimming up the CC list, I don't think this is too relevant to the
wider stable community)
On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
On 1/26/24 12:34, Nathan Chancellor wrote:
On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
On 1/26/24 09:51, Greg Kroah-Hartman wrote:
On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
On 1/22/24 15:55, Greg Kroah-Hartman wrote:
This is the start of the stable review cycle for the 5.10.209 release.
There are 286 patches in this series, all will be posted as a response
to this one. If anyone has any issues with these being applied, please
let me know.
Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
Anything received after that time might be too late.
[ ... ]
zhenwei pi <pizhenwei@xxxxxxxxxxxxx>
virtio-crypto: implement RSA algorithm
Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
It is quite beyond a bug fix. Also, unless I am really missing something,
the series (or at least this patch) was not applied to v5.15.y, so we now
have functionality in v5.10.y which is not in v5.15.y.
See the commit text, it was a dependency of a later fix and documented
as such.
Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
gladly accepted :)
We reverted the entire series from the merge because it results in a build
failure for us.
In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
/home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
__read_overflow2_field(q_size_field, size);
For what it's worth, this is likely self inflicted for chromeos-5.10,
which carries a revert of commit eaafc590053b ("fortify: Explicitly
disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
"fortify: Explicitly disable Clang support""). I don't see the series
that added proper support for clang to fortify in 5.18 that ended with
commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
branch, so this seems somewhat expected.
That explains that ;-). I don't mind if the patches stay in v5.10.y,
we have them reverted anyway.
The revert was a pure process issue, as you may see when looking into
commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
Still, that doesn't explain why the problem exists in 5.18+.
I also see that upstream (starting with 6.1) when trying to build it with clang,
so I guess it is one of those bug-for-bug compatibility things. I really have
no idea what causes it, or why we don't see the problem when building
chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
merging 5.10.209 into it. Making things worse, the problem isn't _always_
seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
I have no idea what triggers the problem.
Have a .config that reproduces it on upstream? I have not personally
seen this warning in my build matrix nor has our continuous-integration
matrix (I don't see it in the warning output at the bottom but that
could have missed something for some reason) in 6.1:
The following command sequence reproduces the problem for me with all stable
branches starting with 5.18.y (plus mainline).
rm -rf /tmp/crypto-build
mkdir /tmp/crypto-build
make -j CC=clang-15 mrproper >/dev/null 2>&1
make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
I tried clang versions 14, 15, and 16. This is with my home system running
Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
the version is
Ubuntu clang version 15.0.7
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Okay interesting, this warning is hidden behind W=1, which our CI does
not test with. Looks like it has been that way since the introduction of
these checks in f68f2ff91512 ("fortify: Detect struct member overflows
in memcpy() at compile-time").
Interestingly the warning is seen in chromeos-5.10, without this patch,
and without W=1. I guess that must have to do with the revert which is
finally biting us.
I think this is a legitimate warning though. It is complaining about the
second memcpy() in virtio_crypto_alg_akcipher_init_session():
memcpy(&ctrl->u, para, sizeof(ctrl->u));
where ctrl is:
struct virtio_crypto_op_ctrl_req {
struct virtio_crypto_ctrl_header header; /* 0 16 */
union {
struct virtio_crypto_sym_create_session_req sym_create_session; /* 16 56 */
struct virtio_crypto_hash_create_session_req hash_create_session; /* 16 56 */
struct virtio_crypto_mac_create_session_req mac_create_session; /* 16 56 */
struct virtio_crypto_aead_create_session_req aead_create_session; /* 16 56 */
struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /* 16 56 */
struct virtio_crypto_destroy_session_req destroy_session; /* 16 56 */
__u8 padding[56]; /* 16 56 */
} u; /* 16 56 */
union {
struct virtio_crypto_sym_create_session_req sym_create_session; /* 0 56 */
struct virtio_crypto_hash_create_session_req hash_create_session; /* 0 56 */
struct virtio_crypto_mac_create_session_req mac_create_session; /* 0 56 */
struct virtio_crypto_aead_create_session_req aead_create_session; /* 0 56 */
struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /* 0 56 */
struct virtio_crypto_destroy_session_req destroy_session; /* 0 56 */
__u8 padding[56]; /* 0 56 */
};
/* size: 72, cachelines: 2, members: 2 */
/* last cacheline: 8 bytes */
};
(so size and p_size_field should be 56) and the type of the para
parameter in virtio_crypto_alg_akcipher_init_session() is 'void *' but
the para passed by reference to
virtio_crypto_alg_akcipher_init_session() in virtio_crypto_rsa_set_key()
has a type of 'struct virtio_crypto_akcipher_session_para':
struct virtio_crypto_akcipher_session_para {
__le32 algo; /* 0 4 */
__le32 keytype; /* 4 4 */
__le32 keylen; /* 8 4 */
union {
struct virtio_crypto_rsa_session_para rsa; /* 12 8 */
struct virtio_crypto_ecdsa_session_para ecdsa; /* 12 8 */
} u; /* 12 8 */
union {
struct virtio_crypto_rsa_session_para rsa; /* 0 8 */
struct virtio_crypto_ecdsa_session_para ecdsa; /* 0 8 */
};
/* size: 20, cachelines: 1, members: 4 */
/* last cacheline: 20 bytes */
};
(so q_size_field would be 20 if clang were able to do inlining to see
through the 'void *'...?), which would result in the
__compiletime_lessthan(q_size_field, size)
check succeeding and triggering the warning because 20 < 56, so it does
seem like there is an overread of the source buffer here? Adding the
Looks like it; I think either the passed 'para' should be of type
virtio_crypto_akcipher_create_session_req() or it should only copy
sizeof(struct virtio_crypto_akcipher_session_para) bytes.
Anyway, how did you find that ? Is there a magic trick to find the
actual code causing the warning ? I am asking because we had seen
similar warnings before, and it would help to know how to find the
problematic code.
Thanks,
Guenter
maintainers of the driver and subsystem in question.
Cheers,
Nathan
