| Message ID | 20170731090438.458392-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
Hi Arnd, I think you are right, but removing this is maybe the wrong fix. The issue is, that CAPI messages are packed byte streams and yes the 64bit extension of CAPI is not very good designed for modern CPU constrains with alignment, since the data pointer for the buffer is not on a 64bit boundary. All hardware controller implementations I know do ignore the data pointer, they are simple awaiting the data of the given length directly after the message. Only the application interface on 64 bit systems really need the 64 bit data pointer value, which is usually set in the HW driver to the mapped user space address of the used data buffer if the user space process is 64bit. The user space CAPI library correctly use byte stream access functions to read/write the values. So the correct solution for this driver would be to use stream access functions as well here and not add a 64 bit pointer. On the other hand I do not think the any people use this driver on 64 bit systems today, because they would run into this issue. Best Karsten Am 31.07.2017 um 11:04 schrieb Arnd Bergmann: > I patched a variant of this warning before, but now saw it come back > in a different configuration with gcc-7 and UBSAN: > > drivers/isdn/hardware/eicon/message.c: In function 'mixer_notify_update': > drivers/isdn/hardware/eicon/message.c:11162:54: error: array subscript is above array bounds [-Werror=array-bounds] > ((CAPI_MSG *) msg)->info.facility_req.structs[1] = LI_REQ_SILENT_UPDATE & 0xff; > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/isdn/hardware/eicon/message.c:11163:54: error: array subscript is above array bounds [-Werror=array-bounds] > ((CAPI_MSG *) msg)->info.facility_req.structs[2] = LI_REQ_SILENT_UPDATE >> 8; > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/isdn/hardware/eicon/message.c:11164:54: error: array subscript is above array bounds [-Werror=array-bounds] > ((CAPI_MSG *) msg)->info.facility_req.structs[3] = 0; > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~ > > I spent a long time narrowing down what caused this, as I suspected > yet another false-positive warning in gcc. However, this time it > turned out to be an ancient kernel bug, which probably prevented > this from ever working on 64-bit machines, causing a stack > buffer overflow as indicated by the warning originally. > > The problem is that having a 64-bit pointer inside of the CAPI_MSG->info > union leads to the start of the union to become 64-bit aligned by adding > four padding bytes. The structure is however aliased to a fixed-length > array on the stack in mixer_notify_update(), and later copied directly > to the hardware, so both go wrong. > > This just removes the fields that were apparently added in a misguided > attempt to make the driver work on 64-bit machines but never actually > used. > > Fixes: 950eabbd6dde ("ISDN: eicon: silence misleading array-bounds warning") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/isdn/hardware/eicon/capi20.h | 18 ------------------ > 1 file changed, 18 deletions(-) > > diff --git a/drivers/isdn/hardware/eicon/capi20.h b/drivers/isdn/hardware/eicon/capi20.h > index 391e4175b0b5..7b97cd576485 100644 > --- a/drivers/isdn/hardware/eicon/capi20.h > +++ b/drivers/isdn/hardware/eicon/capi20.h > @@ -301,14 +301,6 @@ typedef struct { > word Number; > word Flags; > } _DAT_B3_REQP; > -/* DATA-B3-REQUEST 64 BIT Systems */ > -typedef struct { > - dword Data; > - word Data_Length; > - word Number; > - word Flags; > - void *pData; > -} _DAT_B3_REQ64P; > /* DATA-B3-CONFIRM */ > typedef struct { > word Number; > @@ -321,14 +313,6 @@ typedef struct { > word Number; > word Flags; > } _DAT_B3_INDP; > -/* DATA-B3-INDICATION 64 BIT Systems */ > -typedef struct { > - dword Data; > - word Data_Length; > - word Number; > - word Flags; > - void *pData; > -} _DAT_B3_IND64P; > /* DATA-B3-RESPONSE */ > typedef struct { > word Number; > @@ -409,10 +393,8 @@ struct _API_MSG { > _DIS_B3_INDP disconnect_b3_ind; > _DIS_B3_RESP disconnect_b3_res; > _DAT_B3_REQP data_b3_req; > - _DAT_B3_REQ64P data_b3_req64; > _DAT_B3_CONP data_b3_con; > _DAT_B3_INDP data_b3_ind; > - _DAT_B3_IND64P data_b3_ind64; > _DAT_B3_RESP data_b3_res; > _RES_B3_REQP reset_b3_req; > _RES_B3_CONP reset_b3_con; >
On Mon, Jul 31, 2017 at 9:38 PM, <isdn@linux-pingi.de> wrote: > Hi Arnd, > > I think you are right, but removing this is maybe the wrong fix. > > The issue is, that CAPI messages are packed byte streams and yes the > 64bit extension of CAPI is not very good designed for modern CPU > constrains with alignment, since the data pointer for the buffer is not > on a 64bit boundary. I also attempted to just mark the pointer as __packed, which would address the warning as well, but then I saw that no code refers to it. > All hardware controller implementations I know do > ignore the data pointer, they are simple awaiting the data of the given > length directly after the message. Only the application interface on 64 > bit systems really need the 64 bit data pointer value, which is usually > set in the HW driver to the mapped user space address of the used data > buffer if the user space process is 64bit. > The user space CAPI library correctly use byte stream access functions > to read/write the values. So the correct solution for this driver would > be to use stream access functions as well here and not add a 64 bit > pointer. I'm not sure what change you are proposing here. > On the other hand I do not think the any people use this driver on 64 > bit systems today, because they would run into this issue. Right. Another way of addressing the warning would be to add a 'depends on BROKEN || !64BIT' dependency in Kconfig, which would serve to document the fact that it's broken and prevent us from running into it. Arnd
diff --git a/drivers/isdn/hardware/eicon/capi20.h b/drivers/isdn/hardware/eicon/capi20.h index 391e4175b0b5..7b97cd576485 100644 --- a/drivers/isdn/hardware/eicon/capi20.h +++ b/drivers/isdn/hardware/eicon/capi20.h @@ -301,14 +301,6 @@ typedef struct { word Number; word Flags; } _DAT_B3_REQP; -/* DATA-B3-REQUEST 64 BIT Systems */ -typedef struct { - dword Data; - word Data_Length; - word Number; - word Flags; - void *pData; -} _DAT_B3_REQ64P; /* DATA-B3-CONFIRM */ typedef struct { word Number; @@ -321,14 +313,6 @@ typedef struct { word Number; word Flags; } _DAT_B3_INDP; -/* DATA-B3-INDICATION 64 BIT Systems */ -typedef struct { - dword Data; - word Data_Length; - word Number; - word Flags; - void *pData; -} _DAT_B3_IND64P; /* DATA-B3-RESPONSE */ typedef struct { word Number; @@ -409,10 +393,8 @@ struct _API_MSG { _DIS_B3_INDP disconnect_b3_ind; _DIS_B3_RESP disconnect_b3_res; _DAT_B3_REQP data_b3_req; - _DAT_B3_REQ64P data_b3_req64; _DAT_B3_CONP data_b3_con; _DAT_B3_INDP data_b3_ind; - _DAT_B3_IND64P data_b3_ind64; _DAT_B3_RESP data_b3_res; _RES_B3_REQP reset_b3_req; _RES_B3_CONP reset_b3_con;
I patched a variant of this warning before, but now saw it come back in a different configuration with gcc-7 and UBSAN: drivers/isdn/hardware/eicon/message.c: In function 'mixer_notify_update': drivers/isdn/hardware/eicon/message.c:11162:54: error: array subscript is above array bounds [-Werror=array-bounds] ((CAPI_MSG *) msg)->info.facility_req.structs[1] = LI_REQ_SILENT_UPDATE & 0xff; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/isdn/hardware/eicon/message.c:11163:54: error: array subscript is above array bounds [-Werror=array-bounds] ((CAPI_MSG *) msg)->info.facility_req.structs[2] = LI_REQ_SILENT_UPDATE >> 8; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/isdn/hardware/eicon/message.c:11164:54: error: array subscript is above array bounds [-Werror=array-bounds] ((CAPI_MSG *) msg)->info.facility_req.structs[3] = 0; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~ I spent a long time narrowing down what caused this, as I suspected yet another false-positive warning in gcc. However, this time it turned out to be an ancient kernel bug, which probably prevented this from ever working on 64-bit machines, causing a stack buffer overflow as indicated by the warning originally. The problem is that having a 64-bit pointer inside of the CAPI_MSG->info union leads to the start of the union to become 64-bit aligned by adding four padding bytes. The structure is however aliased to a fixed-length array on the stack in mixer_notify_update(), and later copied directly to the hardware, so both go wrong. This just removes the fields that were apparently added in a misguided attempt to make the driver work on 64-bit machines but never actually used. Fixes: 950eabbd6dde ("ISDN: eicon: silence misleading array-bounds warning") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/isdn/hardware/eicon/capi20.h | 18 ------------------ 1 file changed, 18 deletions(-) -- 2.9.0