| Message ID | 20150909161526.2828.55032.stgit@tstruk-mobl1 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Am Mittwoch, 9. September 2015, 09:15:26 schrieb Tadeusz Struk: Hi Tadeusz, >Add sg_len function which returns the total number of bytes in sg. > >Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> >--- > include/linux/scatterlist.h | 1 + > lib/scatterlist.c | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+) > >diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h >index 9b1ef0c..7c82fc1 100644 >--- a/include/linux/scatterlist.h >+++ b/include/linux/scatterlist.h >@@ -246,6 +246,7 @@ static inline void *sg_virt(struct scatterlist *sg) > } > > int sg_nents(struct scatterlist *sg); >+int sg_len(struct scatterlist *sg); > int sg_nents_for_len(struct scatterlist *sg, u64 len); > struct scatterlist *sg_next(struct scatterlist *); > struct scatterlist *sg_last(struct scatterlist *s, unsigned int); >diff --git a/lib/scatterlist.c b/lib/scatterlist.c >index d105a9f..71324bb 100644 >--- a/lib/scatterlist.c >+++ b/lib/scatterlist.c >@@ -57,6 +57,24 @@ int sg_nents(struct scatterlist *sg) > EXPORT_SYMBOL(sg_nents); > > /** >+ * sg_len - return total size of bytes in the scatterlist >+ * @sg: The scatterlist >+ * >+ * Description: >+ * Allows to know how the total size of bytes in sg, taking into acount >+ * chaining as well >+ **/ >+int sg_len(struct scatterlist *sg) unsigned int? >+{ >+ int len; >+ >+ for (len = 0; sg; sg = sg_next(sg)) >+ len += sg->length; >+ return len; >+} >+EXPORT_SYMBOL(sg_len); >+ >+/** > * sg_nents_for_len - return total count of entries in scatterlist > * needed to satisfy the supplied length > * @sg: The scatterlist > >-- >To unsubscribe from this list: send the line "unsubscribe linux-crypto" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2015 09:27 AM, Stephan Mueller wrote: >> +int sg_len(struct scatterlist *sg) > unsigned int? No, because it can return -EINVAL if you call it before you set the key. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 9. September 2015, 09:31:00 schrieb Tadeusz Struk: Hi Tadeusz, >On 09/09/2015 09:27 AM, Stephan Mueller wrote: >>> +int sg_len(struct scatterlist *sg) >> >> unsigned int? > >No, because it can return -EINVAL if you call it before you set the key. I see. But, shouldn't there be an overflow check? Maybe not here, but in the cases where the function is invoked. There is a kmalloc(src_len) without a check for negative values. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2015 09:39 AM, Stephan Mueller wrote: >> No, because it can return -EINVAL if you call it before you set the key. > I see. > > But, shouldn't there be an overflow check? Maybe not here, but in the cases > where the function is invoked. There is a kmalloc(src_len) without a check for > negative values. Right, but because testmgr.c calls setkey before this I skipped the check. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 9. September 2015, 09:46:36 schrieb Tadeusz Struk: Hi Tadeusz, >On 09/09/2015 09:39 AM, Stephan Mueller wrote: >>> No, because it can return -EINVAL if you call it before you set the key. >> >> I see. >> >> But, shouldn't there be an overflow check? Maybe not here, but in the cases >> where the function is invoked. There is a kmalloc(src_len) without a check >> for negative values. > >Right, but because testmgr.c calls setkey before this I skipped the check. But in the rsa.c enc/dec/verify/sign functions, there should be such check, I would guess. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2015 09:49 AM, Stephan Mueller wrote: >>> >> But, shouldn't there be an overflow check? Maybe not here, but in the cases >>> >> where the function is invoked. There is a kmalloc(src_len) without a check >>> >> for negative values. >> > >> >Right, but because testmgr.c calls setkey before this I skipped the check. > But in the rsa.c enc/dec/verify/sign functions, there should be such check, I > would guess. There is see line 419: return pkey->n ? mpi_get_size(pkey->n) : -EINVAL; -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 9. September 2015, 09:31:00 schrieb Tadeusz Struk: Hi Tadeusz, >On 09/09/2015 09:27 AM, Stephan Mueller wrote: >>> +int sg_len(struct scatterlist *sg) >> >> unsigned int? > >No, because it can return -EINVAL if you call it before you set the key. Just re-reading the code: where would the -EINVAL be generated? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 9. September 2015, 09:51:40 schrieb Tadeusz Struk: Hi Tadeusz, >On 09/09/2015 09:49 AM, Stephan Mueller wrote: >>>> >> But, shouldn't there be an overflow check? Maybe not here, but in the >>>> >> cases >>>> >> where the function is invoked. There is a kmalloc(src_len) without a >>>> >> check >>>> >> for negative values. >>> > >>> >Right, but because testmgr.c calls setkey before this I skipped the >>> >check. >> >> But in the rsa.c enc/dec/verify/sign functions, there should be such check, >> I would guess. > >There is see line 419: >return pkey->n ? mpi_get_size(pkey->n) : -EINVAL; I feel we are not talking about the same issue. I refer to your patch in rsa.c: + int src_len = sg_len(req->src), dst_len = sg_len(req->dst); ===> can be negative according to your statement ... + void *ptr = kmalloc(dst_len, GFP_KERNEL); ===> with a negative number, I guess we have a problem here. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2015 09:56 AM, Stephan Mueller wrote: >>> But in the rsa.c enc/dec/verify/sign functions, there should be such check, >>> >> I would guess. >> > >> >There is see line 419: >> >return pkey->n ? mpi_get_size(pkey->n) : -EINVAL; > I feel we are not talking about the same issue. I refer to your patch in > rsa.c: > > + int src_len = sg_len(req->src), dst_len = sg_len(req->dst); > > ===> can be negative according to your statement > > ... > > + void *ptr = kmalloc(dst_len, GFP_KERNEL); > > ===> with a negative number, I guess we have a problem here. Yes, sorry, you are right. sg_len() will only return positive numbers or zero. rsa.c checks it in all four operations: if (unlikely(!pkey->n || !pkey->d || !src_len)) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 9. September 2015, 10:02:17 schrieb Tadeusz Struk: Hi Tadeusz, >On 09/09/2015 09:56 AM, Stephan Mueller wrote: >>>> But in the rsa.c enc/dec/verify/sign functions, there should be such >>>> check, >>>> >>>> >> I would guess. >>> > >>> >There is see line 419: >>> >return pkey->n ? mpi_get_size(pkey->n) : -EINVAL; >> >> I feel we are not talking about the same issue. I refer to your patch in >> rsa.c: >> >> + int src_len = sg_len(req->src), dst_len = sg_len(req->dst); >> >> ===> can be negative according to your statement >> >> ... >> >> + void *ptr = kmalloc(dst_len, GFP_KERNEL); >> >> ===> with a negative number, I guess we have a problem here. > >Yes, sorry, you are right. sg_len() will only return positive numbers or >zero. rsa.c checks it in all four operations: >if (unlikely(!pkey->n || !pkey->d || !src_len)) Great, I am not disputing the check for 0, I just want an unsigned int, because sg->length is unsigned int too. :-) Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2015 10:05 AM, Stephan Mueller wrote: >> Yes, sorry, you are right. sg_len() will only return positive numbers or >> >zero. rsa.c checks it in all four operations: >> >if (unlikely(!pkey->n || !pkey->d || !src_len)) > Great, I am not disputing the check for 0, I just want an unsigned int, > because sg->length is unsigned int too. :-) I see, maybe we can check for negative numbers in PF_ALG? -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 9. September 2015, 10:16:16 schrieb Tadeusz Struk: Hi Tadeusz, >On 09/09/2015 10:05 AM, Stephan Mueller wrote: >>> Yes, sorry, you are right. sg_len() will only return positive numbers or >>> >>> >zero. rsa.c checks it in all four operations: >>> >if (unlikely(!pkey->n || !pkey->d || !src_len)) >> >> Great, I am not disputing the check for 0, I just want an unsigned int, >> because sg->length is unsigned int too. :-) > >I see, maybe we can check for negative numbers in PF_ALG? My request for turning the implementation of sg_len and the callers of it to use unsigned int is simply to avoid overflows of the counter. Note, I usually am very zealous about using the correct data types, especially with integers. I have seen way to many security related bugs by overflowing a signed integer. Surely, PF_ALG will ensure that user space will only provide buffers up to a max number (PAGE_SIZE * ALG_MAX_PAGES is the maximum user space can provide at all considering my current user space approach). So, we have at most 65536 bytes from user space in one request. This boundary is to allow at most ALG_MAX_PAGES individual SGL members (i.e. at most ALG_MAX_PAGES individual calls to splice) but also tries to squeeze the data coming with sendmsg into one page. But, surely we can discuss these limits once I post algif_akcipher. Considering that, I do not feel that the code we discuss here should have a check for the maximum size of the SGL. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 9b1ef0c..7c82fc1 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -246,6 +246,7 @@ static inline void *sg_virt(struct scatterlist *sg) } int sg_nents(struct scatterlist *sg); +int sg_len(struct scatterlist *sg); int sg_nents_for_len(struct scatterlist *sg, u64 len); struct scatterlist *sg_next(struct scatterlist *); struct scatterlist *sg_last(struct scatterlist *s, unsigned int); diff --git a/lib/scatterlist.c b/lib/scatterlist.c index d105a9f..71324bb 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -57,6 +57,24 @@ int sg_nents(struct scatterlist *sg) EXPORT_SYMBOL(sg_nents); /** + * sg_len - return total size of bytes in the scatterlist + * @sg: The scatterlist + * + * Description: + * Allows to know how the total size of bytes in sg, taking into acount + * chaining as well + **/ +int sg_len(struct scatterlist *sg) +{ + int len; + + for (len = 0; sg; sg = sg_next(sg)) + len += sg->length; + return len; +} +EXPORT_SYMBOL(sg_len); + +/** * sg_nents_for_len - return total count of entries in scatterlist * needed to satisfy the supplied length * @sg: The scatterlist
Add sg_len function which returns the total number of bytes in sg. Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> --- include/linux/scatterlist.h | 1 + lib/scatterlist.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html