cifs: small underflow in cnvrtDosUnixTm()
Subsystems (3)
PATCH 1/1 cifs: small underflow in cnvrtDosUnixTm()
January is month 1. There is no zero-th month. We don't care very much if the days are invalid but for months, we use it to read from an array so this bug means we read one space before the start of the total_days_of_prev_months[] array.
fs/cifs/netmisc.c +3 -1
@@ -980,8 +980,10 @@ struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset)cifs_dbg (VFS ,"illegal hours %d\n" , st->Hours); days = sd->Day; month = sd->Month; -if ((days >31 ) || (month >12 )) { +if (days >31 || month <1 || month >12 ) {cifs_dbg (VFS ,"illegal date, month %d day: %d\n" , month, days); +if (month <1 ) + month =1 ;if (month >12 ) month =12 ; }
PATCH v2 1/1 cifs: small underflow in cnvrtDosUnixTm()
January is month 1. There is no zero-th month. If someone passes a zero month then it means we read from one space before the start of the total_days_of_prev_months[] array.
We may as well also be strict about days as well.
---
v2: Be strict about days as well. My first patch was less intrusive because it only prevented the out of bounds access. I have no idea how common it is to pass in an illegal day but, hopefully, not very common.
fs/cifs/netmisc.c +3 -3
@@ -980,10 +980,10 @@ struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset)cifs_dbg (VFS ,"illegal hours %d\n" , st->Hours); days = sd->Day; month = sd->Month; -if ((days >31 ) || (month >12 )) { +if (days <1 || days >31 || month <1 || month >12 ) {cifs_dbg (VFS ,"illegal date, month %d day: %d\n" , month, days); -if (month >12 ) - month =12 ; + days = (days <1 ) ?1 : ((days <=31 ) ? days :31 ); + month = (month <1 ) ?1 : ((month <=12 ) ? month :12 ); } month -=1 ; days += total_days_of_prev_months[month];
Am 28.04.2017 14:51, schrieb Dan Carpenter:
Quoted 21 lines
January is month 1. There is no zero-th month. If someone passes a zero month then it means we read from one space before the start of the total_days_of_prev_months[] array.
We may as well also be strict about days as well.
Fixes 1bd5bbcb6531 ("[CIFS] Legacy time handling for Win9x and OS/2 part 1")Signed-off-by Dan Carpenter <[email protected]>---
v2: Be strict about days as well. My first patch was less intrusive because it only prevented the out of bounds access. I have no idea how common it is to pass in an illegal day but, hopefully, not very common.fs/cifs/netmisc.c +0
-0@@ -980,10 +980,10 @@ struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset)cifs_dbg (VFS ,"illegal hours %d\n" , st->Hours); days = sd->Day; month = sd->Month;
fs/cifs/netmisc.c struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset) · line 989
Quoted 10 lines
fs/cifs/netmisc.c +3
-3-if ((days >31 ) || (month >12 )) { +if (days <1 || days >31 || month <1 || month >12 ) {cifs_dbg (VFS ,"illegal date, month %d day: %d\n" , month, days); -if (month >12 ) - month =12 ; + days = (days <1 ) ?1 : ((days <=31 ) ? days :31 ); + month = (month <1 ) ?1 : ((month <=12 ) ? month :12 ); } month -=1 ; days += total_days_of_prev_months[month];
The mixing in now a bit unfortunate ... why not simply
if (days < 1 || days > 31 || month < 1 || month > 12) cifs_dbg(VFS, "illegal date, month %d day: %d\n", month, days); month = (month < 1) ? 1 : ((month <= 12) ? month : 12); days = (days < 1) ? 1 : ((days <= 31) ? days : 31);
hope that helps,
re,
wh
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 37 lines · walter harms
Am 28.04.2017 14:51, schrieb Dan Carpenter:
Quoted 21 lines
January is month 1. There is no zero-th month. If someone passes a zero month then it means we read from one space before the start of the total_days_of_prev_months[] array.
We may as well also be strict about days as well.
Fixes 1bd5bbcb6531 ("[CIFS] Legacy time handling for Win9x and OS/2 part 1")Signed-off-by Dan Carpenter <[email protected]>---
v2: Be strict about days as well. My first patch was less intrusive because it only prevented the out of bounds access. I have no idea how common it is to pass in an illegal day but, hopefully, not very common.fs/cifs/netmisc.c +0
-0@@ -980,10 +980,10 @@ struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset)cifs_dbg (VFS ,"illegal hours %d\n" , st->Hours); days = sd->Day; month = sd->Month;fs/cifs/netmisc.c struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset) · line 989
Quoted 10 lines
fs/cifs/netmisc.c +3
-3-if ((days >31 ) || (month >12 )) { +if (days <1 || days >31 || month <1 || month >12 ) {cifs_dbg (VFS ,"illegal date, month %d day: %d\n" , month, days); -if (month >12 ) - month =12 ; + days = (days <1 ) ?1 : ((days <=31 ) ? days :31 ); + month = (month <1 ) ?1 : ((month <=12 ) ? month :12 ); } month -=1 ; days += total_days_of_prev_months[month];The mixing in now a bit unfortunate ... why not simply
Quoted 6 lines
if (days < 1 || days > 31 || month < 1 || month > 12) cifs_dbg(VFS, "illegal date, month %d day: %d\n", month, days); month = (month < 1) ? 1 : ((month <= 12) ? month : 12); days = (days < 1) ? 1 : ((days <= 31) ? days : 31);
I prefer my version because I feel like it more closely expresses what I want to say.
regards,
dan carpenter
Am 28.04.2017 16:41, schrieb Dan Carpenter:
Quoted 41 lines · walter harms
Quoted 40 lines
Am 28.04.2017 14:51, schrieb Dan Carpenter:
Quoted 21 lines
January is month 1. There is no zero-th month. If someone passes a zero month then it means we read from one space before the start of the total_days_of_prev_months[] array.
We may as well also be strict about days as well.
Fixes 1bd5bbcb6531 ("[CIFS] Legacy time handling for Win9x and OS/2 part 1")Signed-off-by Dan Carpenter <[email protected]>---
v2: Be strict about days as well. My first patch was less intrusive because it only prevented the out of bounds access. I have no idea how common it is to pass in an illegal day but, hopefully, not very common.fs/cifs/netmisc.c +0
-0@@ -980,10 +980,10 @@ struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset)cifs_dbg (VFS ,"illegal hours %d\n" , st->Hours); days = sd->Day; month = sd->Month;fs/cifs/netmisc.c struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, int offset) · line 989
Quoted 10 lines
fs/cifs/netmisc.c +3
-3-if ((days >31 ) || (month >12 )) { +if (days <1 || days >31 || month <1 || month >12 ) {cifs_dbg (VFS ,"illegal date, month %d day: %d\n" , month, days); -if (month >12 ) - month =12 ; + days = (days <1 ) ?1 : ((days <=31 ) ? days :31 ); + month = (month <1 ) ?1 : ((month <=12 ) ? month :12 ); } month -=1 ; days += total_days_of_prev_months[month];The mixing in now a bit unfortunate ... why not simply
if (days < 1 || days > 31 || month < 1 || month > 12) cifs_dbg(VFS, "illegal date, month %d day: %d\n", month, days);
Quoted 7 lines
Quoted 3 lines
month = (month < 1) ? 1 : ((month <= 12) ? month : 12); days = (days < 1) ? 1 : ((days <= 31) ? days : 31);I prefer my version because I feel like it more closely expresses what I want to say.
(Actually it is your code just without the braces.) what is about differentiating between day and month ?
if (days < 1 || days > 31) {
cifs_dbg(VFS, "illegal day: %d\n", days);
days = (days < 1) ? 1 : 31 ;
}if ( month < 1 || month > 12) {
cifs_dbg(VFS, "illegal month %d\n", month);
month = (month < 1) ? 1 : 12 ;
}this way you have a obvious error handling for day and month.
I still like my first version even when day/month are recalculated every time the flow is very linear only the error message is a deviation. (Given that ?: makes it not easy to digest)
re,
wh
A message that is not in the archive (sent off-list or not yet received): [email protected]
I will resend on Monday.
regards,
dan carpenter