crypto: user - Allow get request with empty driver name
diff mbox series

Message ID 20141120044650.GA28691@gondor.apana.org.au
State New, archived
Headers show
Series
  • crypto: user - Allow get request with empty driver name
Related show

Commit Message

Herbert Xu Nov. 20, 2014, 4:46 a.m. UTC
On Thu, Nov 20, 2014 at 05:23:23AM +0100, Stephan Mueller wrote:
> 
> Here is the code:
> 
> static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
> 			 struct nlattr **attrs)
> {
> ...
> 	if (!p->cru_driver_name[0])
> 		return -EINVAL;

OK let's fix this.

crypto: user - Allow get request with empty driver name
    
Currently all get requests with an empty driver name fail with
EINVAL.  Since most users actually want to supply an empty driver
name this patch removes this check.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,

Comments

Steffen Klassert Nov. 20, 2014, 7:11 a.m. UTC | #1
On Thu, Nov 20, 2014 at 12:46:50PM +0800, Herbert Xu wrote:
> On Thu, Nov 20, 2014 at 05:23:23AM +0100, Stephan Mueller wrote:
> > 
> > Here is the code:
> > 
> > static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
> > 			 struct nlattr **attrs)
> > {
> > ...
> > 	if (!p->cru_driver_name[0])
> > 		return -EINVAL;
> 
> OK let's fix this.
> 
> crypto: user - Allow get request with empty driver name
>     
> Currently all get requests with an empty driver name fail with
> EINVAL.  Since most users actually want to supply an empty driver
> name this patch removes this check.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index e2a34fe..0bb30ac 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -201,10 +201,7 @@ static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
>  	if (!null_terminated(p->cru_name) || !null_terminated(p->cru_driver_name))
>  		return -EINVAL;
>  
> -	if (!p->cru_driver_name[0])
> -		return -EINVAL;
> -
> -	alg = crypto_alg_match(p, 1);
> +	alg = crypto_alg_match(p, 0);

I think this is not sufficient, crypto_alg_match() will now return the first
algorithm in crypto_alg_list that matches cra_name. We would need to extend
crypto_alg_match() to return the algorithm with the highest priority in that
case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Herbert Xu Nov. 20, 2014, 7:45 a.m. UTC | #2
On Thu, Nov 20, 2014 at 08:11:42AM +0100, Steffen Klassert wrote:
>
> I think this is not sufficient, crypto_alg_match() will now return the first
> algorithm in crypto_alg_list that matches cra_name. We would need to extend
> crypto_alg_match() to return the algorithm with the highest priority in that
> case.

It doesn't really matter because all implementations must provide
exactly the same set of parameters for a given algorithm so it's
good enough for what Stephan wants to do.

But yes an interface to grab the highest priority algorithm would
be useful too so patches are welcome :)

Cheers,
Steffen Klassert Nov. 20, 2014, 8:04 a.m. UTC | #3
On Thu, Nov 20, 2014 at 03:45:26PM +0800, Herbert Xu wrote:
> On Thu, Nov 20, 2014 at 08:11:42AM +0100, Steffen Klassert wrote:
> >
> > I think this is not sufficient, crypto_alg_match() will now return the first
> > algorithm in crypto_alg_list that matches cra_name. We would need to extend
> > crypto_alg_match() to return the algorithm with the highest priority in that
> > case.
> 
> It doesn't really matter because all implementations must provide
> exactly the same set of parameters for a given algorithm so it's
> good enough for what Stephan wants to do.

Ok, I see.

> But yes an interface to grab the highest priority algorithm would
> be useful too so patches are welcome :)

Should be not too hard to return the highest priority algorithm
instead of the first match with the existing interface, I'll see
what I can do.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Stephan Mueller Nov. 20, 2014, 1:02 p.m. UTC | #4
Am Donnerstag, 20. November 2014, 12:46:50 schrieb Herbert Xu:

Hi Herbert,

> On Thu, Nov 20, 2014 at 05:23:23AM +0100, Stephan Mueller wrote:
> > Here is the code:
> > 
> > static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
> > 
> > 			 struct nlattr **attrs)
> > 
> > {
> > ...
> > 
> > 	if (!p->cru_driver_name[0])
> > 	
> > 		return -EINVAL;
> 
> OK let's fix this.
> 
> crypto: user - Allow get request with empty driver name
> 
> Currently all get requests with an empty driver name fail with
> EINVAL.  Since most users actually want to supply an empty driver
> name this patch removes this check.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Stephan Mueller <smueller@chronox.de>
> 
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index e2a34fe..0bb30ac 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -201,10 +201,7 @@ static int crypto_report(struct sk_buff *in_skb, struct
> nlmsghdr *in_nlh, if (!null_terminated(p->cru_name) ||
> !null_terminated(p->cru_driver_name)) return -EINVAL;
> 
> -	if (!p->cru_driver_name[0])
> -		return -EINVAL;
> -
> -	alg = crypto_alg_match(p, 1);
> +	alg = crypto_alg_match(p, 0);
>  	if (!alg)
>  		return -ENOENT;
> 
> Cheers,
Stephan Mueller Nov. 20, 2014, 1:07 p.m. UTC | #5
Am Donnerstag, 20. November 2014, 09:04:06 schrieb Steffen Klassert:

Hi Steffen,

> On Thu, Nov 20, 2014 at 03:45:26PM +0800, Herbert Xu wrote:
> > On Thu, Nov 20, 2014 at 08:11:42AM +0100, Steffen Klassert wrote:
> > > I think this is not sufficient, crypto_alg_match() will now return the
> > > first algorithm in crypto_alg_list that matches cra_name. We would need
> > > to extend crypto_alg_match() to return the algorithm with the highest
> > > priority in that case.
> > 
> > It doesn't really matter because all implementations must provide
> > exactly the same set of parameters for a given algorithm so it's
> > good enough for what Stephan wants to do.
> 
> Ok, I see.
> 
> > But yes an interface to grab the highest priority algorithm would
> > be useful too so patches are welcome :)
> 
> Should be not too hard to return the highest priority algorithm
> instead of the first match with the existing interface, I'll see
> what I can do.

I think that for the purpose of using crypto-user in an AF_ALG scenario, even 
searching for the highest priporty will not necessarily give you the reference 
to the cipher used in AF_ALG. In the time between AF_ALG initialized a cipher 
and the time crypto-user is called, a new driver could register that may have 
even a higher priority than the one driver currently active for AF_ALG.

Therefore, from my perspective of AF_ALG and considering that all drivers of 
the same name should be identical, I would not suggest add more code to 
resolve the highest priority as I do not see the value of it.
Stephan Mueller Nov. 20, 2014, 1:10 p.m. UTC | #6
Am Donnerstag, 20. November 2014, 14:02:21 schrieb Stephan Mueller:

Hi Stephan,

> Am Donnerstag, 20. November 2014, 12:46:50 schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Thu, Nov 20, 2014 at 05:23:23AM +0100, Stephan Mueller wrote:
> > > Here is the code:
> > > 
> > > static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr
> > > *in_nlh,
> > > 
> > > 			 struct nlattr **attrs)
> > > 
> > > {
> > > ...
> > > 
> > > 	if (!p->cru_driver_name[0])
> > > 	
> > > 		return -EINVAL;
> > 
> > OK let's fix this.
> > 
> > crypto: user - Allow get request with empty driver name
> > 
> > Currently all get requests with an empty driver name fail with
> > EINVAL.  Since most users actually want to supply an empty driver
> > name this patch removes this check.
> > 
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Acked-by: Stephan Mueller <smueller@chronox.de>

Although I acked the patch, it just occurred that your change modifies the 
user space interface such that you now cannot use a driver name any more.
> 
> > diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> > index e2a34fe..0bb30ac 100644
> > --- a/crypto/crypto_user.c
> > +++ b/crypto/crypto_user.c
> > @@ -201,10 +201,7 @@ static int crypto_report(struct sk_buff *in_skb,
> > struct nlmsghdr *in_nlh, if (!null_terminated(p->cru_name) ||
> > !null_terminated(p->cru_driver_name)) return -EINVAL;
> > 
> > -	if (!p->cru_driver_name[0])
> > -		return -EINVAL;
> > -
> > -	alg = crypto_alg_match(p, 1);
> > +	alg = crypto_alg_match(p, 0);

What about the following

if (p->cru_driver_name[0]
	alg = crypto_alg_match(p, 1);
else
	alg = crypto_alg_match(p, 0);

> > 
> >  	if (!alg)
> >  	
> >  		return -ENOENT;
> > 
> > Cheers,
Herbert Xu Nov. 20, 2014, 1:40 p.m. UTC | #7
On Thu, Nov 20, 2014 at 02:10:00PM +0100, Stephan Mueller wrote:
>
> What about the following
> 
> if (p->cru_driver_name[0]
> 	alg = crypto_alg_match(p, 1);
> else
> 	alg = crypto_alg_match(p, 0);

If cru_driver_name is not empty then exact will never be used, no?

Cheers,
Stephan Mueller Nov. 20, 2014, 4:08 p.m. UTC | #8
Am Donnerstag, 20. November 2014, 21:40:29 schrieb Herbert Xu:

Hi Herbert,

> On Thu, Nov 20, 2014 at 02:10:00PM +0100, Stephan Mueller wrote:
> > What about the following
> > 
> > if (p->cru_driver_name[0]

If the driver name is not empty
> > 
> > 	alg = crypto_alg_match(p, 1);

Do an exact match using the driver name
> > 
> > else
> > 
> > 	alg = crypto_alg_match(p, 0);

Otherise use the generic name for a fuzzy match.
> 
> If cru_driver_name is not empty then exact will never be used, no?

If the code suggestion does not follow my words outlined above, I want my 
words to be considered ;-)

Yet, I am unsure where the code deviates from my words.

> 
> Cheers,
Herbert Xu Nov. 21, 2014, 2:31 a.m. UTC | #9
On Thu, Nov 20, 2014 at 05:08:42PM +0100, Stephan Mueller wrote:
> Am Donnerstag, 20. November 2014, 21:40:29 schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Thu, Nov 20, 2014 at 02:10:00PM +0100, Stephan Mueller wrote:
> > > What about the following
> > > 
> > > if (p->cru_driver_name[0]
> 
> If the driver name is not empty
> > > 
> > > 	alg = crypto_alg_match(p, 1);
> 
> Do an exact match using the driver name
> > > 
> > > else
> > > 
> > > 	alg = crypto_alg_match(p, 0);
> 
> Otherise use the generic name for a fuzzy match.
> > 
> > If cru_driver_name is not empty then exact will never be used, no?
> 
> If the code suggestion does not follow my words outlined above, I want my 
> words to be considered ;-)
> 
> Yet, I am unsure where the code deviates from my words.

No I am asking how is this different from my patch?

Cheers,
Stephan Mueller Nov. 21, 2014, 2:42 a.m. UTC | #10
Am Freitag, 21. November 2014, 10:31:31 schrieb Herbert Xu:

Hi Herbert,

> On Thu, Nov 20, 2014 at 05:08:42PM +0100, Stephan Mueller wrote:
> > Am Donnerstag, 20. November 2014, 21:40:29 schrieb Herbert Xu:
> > 
> > Hi Herbert,
> > 
> > > On Thu, Nov 20, 2014 at 02:10:00PM +0100, Stephan Mueller wrote:
> > > > What about the following
> > > > 
> > > > if (p->cru_driver_name[0]
> > 
> > If the driver name is not empty
> > 
> > > > 	alg = crypto_alg_match(p, 1);
> > 
> > Do an exact match using the driver name
> > 
> > > > else
> > > > 
> > > > 	alg = crypto_alg_match(p, 0);
> > 
> > Otherise use the generic name for a fuzzy match.
> > 
> > > If cru_driver_name is not empty then exact will never be used, no?
> > 
> > If the code suggestion does not follow my words outlined above, I want my
> > words to be considered ;-)
> > 
> > Yet, I am unsure where the code deviates from my words.
> 
> No I am asking how is this different from my patch?

Sorry, I withdraw my comment. Your patch is good as is after rechecking 
crypto_alg_match.

> 
> Cheers,
Stephan Mueller Nov. 21, 2014, 4:40 a.m. UTC | #11
Am Donnerstag, 20. November 2014, 14:02:21 schrieb Stephan Mueller:

Hi Stephan,

> Am Donnerstag, 20. November 2014, 12:46:50 schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Thu, Nov 20, 2014 at 05:23:23AM +0100, Stephan Mueller wrote:
> > > Here is the code:
> > > 
> > > static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr
> > > *in_nlh,
> > > 
> > > 			 struct nlattr **attrs)
> > > 
> > > {
> > > ...
> > > 
> > > 	if (!p->cru_driver_name[0])
> > > 	
> > > 		return -EINVAL;
> > 
> > OK let's fix this.
> > 
> > crypto: user - Allow get request with empty driver name
> > 
> > Currently all get requests with an empty driver name fail with
> > EINVAL.  Since most users actually want to supply an empty driver
> > name this patch removes this check.
> > 
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Acked-by: Stephan Mueller <smueller@chronox.de>
> 
> > diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> > index e2a34fe..0bb30ac 100644
> > --- a/crypto/crypto_user.c
> > +++ b/crypto/crypto_user.c
> > @@ -201,10 +201,7 @@ static int crypto_report(struct sk_buff *in_skb,
> > struct nlmsghdr *in_nlh, if (!null_terminated(p->cru_name) ||
> > !null_terminated(p->cru_driver_name)) return -EINVAL;
> > 
> > -	if (!p->cru_driver_name[0])
> > -		return -EINVAL;
> > -
> > -	alg = crypto_alg_match(p, 1);
> > +	alg = crypto_alg_match(p, 0);

Btw: I tested that patch and it works as expected.

> > 
> >  	if (!alg)
> >  	
> >  		return -ENOENT;
> > 
> > Cheers,

Patch
diff mbox series

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index e2a34fe..0bb30ac 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -201,10 +201,7 @@  static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
 	if (!null_terminated(p->cru_name) || !null_terminated(p->cru_driver_name))
 		return -EINVAL;
 
-	if (!p->cru_driver_name[0])
-		return -EINVAL;
-
-	alg = crypto_alg_match(p, 1);
+	alg = crypto_alg_match(p, 0);
 	if (!alg)
 		return -ENOENT;