Re: [PATCH 4/9] X.509: fix BUG_ON() when hash algorithm is unsupported

[Eric Biggers]

Hi David,

On Thu, Feb 08, 2018 at 03:07:30PM +, David Howells wrote:
> Eric Biggers  wrote:
> 
> > The X.509 parser mishandles the case where the certificate's signature's
> > hash algorithm is not available in the crypto API.  In this case,
> > x509_get_sig_params() doesn't allocate the cert->sig->digest buffer; this
> > part seems to be intentional.
> 
> Well, yes, that would be intentional: we can't digest the digestibles without
> access to a hash algorithm to do so and we can't allocate a digest buffer
> without knowing how big it should be.
> 
> > Fix this by making public_key_verify_signature() return -ENOPKG if the
> > hash buffer has not been allocated.
> 
> Hmmm...  I'm not sure that this is the right place to do this, since it
> obscures a potential invalid argument within the kernel.
> 
> I'm more inclined that the users of X.509 certs should check
> x509->unsupported_sig (pkcs7_verify_sig_chain() does this already partially).
> 

Well, either way using BUG_ON() is inappropriate for recoverable problems where
an error code can be returned.  In this case we can simply indicate that the
signature verification failed.  Note that unprivileged users can reach this
BUG_ON(), and it also appears to be reachable in at least 3 different ways...
So it really has to be fixed.

And considering that the ->unsupported_sig and ->unsupported_key flags are
almost completely broken anyway, it sounds like there would have to be a second
patchset to actually fix them.  So I'd rather you just took the more important
fixes in patches 1-5 now as-is, and then a patchset to make certificates with
unsupported algorithms be accepted in more cases can be done separately.

Thanks,

Eric

RE: [Crypto v6 03/12] tls: support for inline tls

[Atul Gupta]

-Original Message-
From: linux-crypto-ow...@vger.kernel.org 
[mailto:linux-crypto-ow...@vger.kernel.org] On Behalf Of David Miller
Sent: Tuesday, February 20, 2018 9:46 PM
To: Atul Gupta 
Cc: davejwat...@fb.com; herb...@gondor.apana.org.au; s...@queasysnail.net; 
linux-crypto@vger.kernel.org; net...@vger.kernel.org; Ganesh GR 

Subject: Re: [Crypto v6 03/12] tls: support for inline tls

From: Atul Gupta 
Date: Mon, 19 Feb 2018 12:19:41 +0530

> + struct net_device *netdev = NULL;
> +
> + netdev = dev_get_by_index(sock_net(sk), inet->cork.fl.flowi_oif);

No need for an assignment in the variable declaration here.
You immediately set it to something else unconditionally.
Sure.

> +static int get_tls_offload_dev(struct sock *sk) {
> + struct net_device *netdev;
> + struct tls_device *dev;
> + int rc = 0;
> +
> + netdev = get_netdev(sk);
> + if (!netdev)
> + return -EINVAL;
> +
> + mutex_lock(_mutex);
> + list_for_each_entry(dev, _list, dev_list) {
> + if (dev->netdev && dev->netdev(dev, netdev)) {
> + rc = -EEXIST;
> + break;
> + }
> + }
> + mutex_unlock(_mutex);
> + dev_put(netdev);
> + return rc;
> +}

This is really a confusing function.

It's name suggests that it "gets" the offload device.  In that case, if it is 
found it should return success.  Instead we get an -EEXIST error in that case.  
And it returns 0 if not found.
Will rename to no_tls_offload_dev, return 0 is used as default condition in 
calling function.

Better to make this do what it says it does, which would be to return '0' when 
the device is found and return -ENODEV when it is not found.

> + tcp_prot.unhash(sk);
Done

Do not force this to the ipv4 TCP instance, use the pointer through the socket 
to call the proper unhash method.

> + err = tcp_prot.hash(sk);
Done

Likewise.
Thanks

Re: [Crypto v6 03/12] tls: support for inline tls

[David Miller]

From: Atul Gupta 
Date: Mon, 19 Feb 2018 12:19:41 +0530

> + struct net_device *netdev = NULL;
> +
> + netdev = dev_get_by_index(sock_net(sk), inet->cork.fl.flowi_oif);

No need for an assignment in the variable declaration here.
You immediately set it to something else unconditionally.

> +static int get_tls_offload_dev(struct sock *sk)
> +{
> + struct net_device *netdev;
> + struct tls_device *dev;
> + int rc = 0;
> +
> + netdev = get_netdev(sk);
> + if (!netdev)
> + return -EINVAL;
> +
> + mutex_lock(_mutex);
> + list_for_each_entry(dev, _list, dev_list) {
> + if (dev->netdev && dev->netdev(dev, netdev)) {
> + rc = -EEXIST;
> + break;
> + }
> + }
> + mutex_unlock(_mutex);
> + dev_put(netdev);
> + return rc;
> +}

This is really a confusing function.

It's name suggests that it "gets" the offload device.  In that case,
if it is found it should return success.  Instead we get an -EEXIST
error in that case.  And it returns 0 if not found.

Better to make this do what it says it does, which would be to return
'0' when the device is found and return -ENODEV when it is not found.

> + tcp_prot.unhash(sk);

Do not force this to the ipv4 TCP instance, use the pointer through
the socket to call the proper unhash method.

> + err = tcp_prot.hash(sk);

Likewise.

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/20/2018 12:34 PM, Herbert Xu wrote:
> On Mon, Feb 19, 2018 at 01:16:30PM +, Horia Geantă wrote:
>>
>>> And what about ALGIF path from user space ?
>>> What if the user never calls the last sendmsg() which will call 
>>> hash_finup() ?
>>>
>> User is expected to follow the rules of the crypto API.
>> Of course, kernel won't (or at least shouldn't) crash in case of misuse.
>> However, in these cases some resources might not be freed - it's unavoidable.
> 
> the crypto API does not require the presence of a finalisation.
> It is entirely optional.  So leaving resources pinned down until
> final/finup occurs is unacceptable, both from user-space and the
> kernel.
> 
If final/finup is optional, how is the final hash supposed to be retrieved?

According to documentation, these are the accepted flows (with the option to
export/import a partial hash b/w update and final/finup):

.init() -> .update() -> .final()
^| |
'' '---> HASH

.init() -> .update() -> .finup()
^| |
'' '---> HASH

   .digest()
   |
   '---> HASH

Note that digest() is not an issue in the case we are discussing, since resource
allocation happens only in init().

Thanks,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Mon, Feb 19, 2018 at 01:16:30PM +, Horia Geantă wrote:
>
> > And what about ALGIF path from user space ?
> > What if the user never calls the last sendmsg() which will call 
> > hash_finup() ?
> > 
> User is expected to follow the rules of the crypto API.
> Of course, kernel won't (or at least shouldn't) crash in case of misuse.
> However, in these cases some resources might not be freed - it's unavoidable.

the crypto API does not require the presence of a finalisation.
It is entirely optional.  So leaving resources pinned down until
final/finup occurs is unacceptable, both from user-space and the
kernel.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt