| Message ID | ZvEFQAWVgWNd9j7e@gondor.apana.org.au |
|---|---|
| State | New |
| Headers | show |
| Series | hwrng: core - Add WARN_ON for buggy read return values | expand |
On Mon Sep 23, 2024 at 5:31 PM EEST, Jarkko Sakkinen wrote: > Greg, did we have something under Documentation/ that would fully > address the use of WARN? ... personally I think that even if there was a separate document, this should be somehow addressed already in SubmittingPatches so that it can't be possibly missed by anyone. BR, Jarkko
On Mon, Sep 23, 2024 at 04:48:27PM +0200, Greg KH wrote: > > Please see: > https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on > which describes that. We should make it more explicit that any WARN() > or WARN_ON() calls that can be hit by user interactions somehow, will > end up getting a CVE id when we fix it up to not do so. If the aformentioned WARN_ON hits, then the driver has probabaly already done a buffer overrun so it's a CVE anyway. Cheers,
On Tue Sep 24, 2024 at 1:32 AM EEST, Herbert Xu wrote: > On Mon, Sep 23, 2024 at 04:48:27PM +0200, Greg KH wrote: > > > > Please see: > > https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on > > which describes that. We should make it more explicit that any WARN() > > or WARN_ON() calls that can be hit by user interactions somehow, will > > end up getting a CVE id when we fix it up to not do so. > > If the aformentioned WARN_ON hits, then the driver has probabaly > already done a buffer overrun so it's a CVE anyway. We'll see I finally got into testing this. Sorry for latencies, I'm switching jobs and unfortunately German Post Office lost my priority mail containing contracts (sent them from Finland to Berlin) so have been signing, scanning etc. the whole day :-) My last week in the current job, and next week is the first in the new job, so this week is a bit bumpy. BR, Jarkko
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 57c51efa5613..018316f54621 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -181,8 +181,15 @@ static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size, int present; BUG_ON(!mutex_is_locked(&reading_mutex)); - if (rng->read) - return rng->read(rng, (void *)buffer, size, wait); + if (rng->read) { + int err; + + err = rng->read(rng, buffer, size, wait); + if (WARN_ON_ONCE(err > 0 && err > size)) + err = size; + + return err; + } if (rng->data_present) present = rng->data_present(rng, wait);
Dear TPM maintainers: Please have a look at the tpm hwrng driver because it appears to be returning a length longer than the buffer length that we gave it. In particular, tpm2 appears to be the culprit (though I didn't really check tpm1 at all so it could also be buggy). The following patch hopefully should confirm that this is indeed caused by TPM and not some other HWRNG driver. ---8<--- If a buggy driver returns a length that is longer than the size of the buffer provided to it, then this may lead to a buffer overread in the caller. Stop this by adding a check for it in the hwrng core. Reported-by: Guangwu Zhang <guazhang@redhat.com> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>