[PATCH] xyzModem: Correct xmodem blk verification conditions
When the blk sequence number is 255 and cblk is 0, the original XOR condition produces a result of 0,and the judgment condition will be unsuccessful.
Signed-off-by: Hongbin Ji jhb_ee@163.com --- common/xyzModem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/xyzModem.c b/common/xyzModem.c index fb319f7119..0bc1a87067 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -373,7 +373,7 @@ xyzModem_get_hdr (void) } ZM_DEBUG (zm_dump (__LINE__)); /* Validate the message */ - if ((xyz.blk ^ xyz.cblk) != (unsigned char) 0xFF) + if (~xyz.blk != xyz.cblk) { ZM_DEBUG (zm_dprintf ("Framing error - blk: %x/%x/%x\n", xyz.blk, xyz.cblk,
On Mon, Feb 05, 2024 at 02:58:50PM +0800, Hongbin Ji wrote:
When the blk sequence number is 255 and cblk is 0, the original XOR condition produces a result of 0,and the judgment condition will be unsuccessful.
This code is 18 years old so it's surprising that it's causing an issue now. This was added in commit f2841d377060 ("Add support for ymodem protocol (loady command). Patch by Stefano Babic, 29 Mar 2006").
Not just 255 and zero but any time "blk == 255 - cblk" then your new condition will be true where the original condition was false. Is that really intentional? Is this from reviewing documentation or something? What documents? Or is it from testing?
regards, dan carpenter
It may be that there are relatively few people using this function, or the length of the transmitted data is less than 128byte * 255 byte, so the triggering conditions for the above problems are not encountered;
I actually discovered this problem during testing. Continuous transmission always fails at block 255.
The following is the protocol's definition of blk cblk, cblk = 255 - blk The original content of the agreement (https://www.menie.org/georges/embedded/xmodem_specification.html) is as follows: -------- 3. MESSAGE BLOCK LEVEL PROTOCOL Each block of the transfer looks like: <SOH><blk #><255-blk #><--128 data bytes--><cksum> in which: <SOH> = 01 hex <blk #> = binary number, starts at 01 increments by 1, and wraps 0FFH to 00H (not to 01) <255-blk #> = blk # after going thru 8080 "CMA" instr, i.e. each bit complemented in the 8-bit block number. Formally, this is the "ones complement".
<cksum> = the sum of the data bytes only. Toss any carry.
在 2024/2/5 22:25, Dan Carpenter 写道:
On Mon, Feb 05, 2024 at 02:58:50PM +0800, Hongbin Ji wrote:
When the blk sequence number is 255 and cblk is 0, the original XOR condition produces a result of 0,and the judgment condition will be unsuccessful.
This code is 18 years old so it's surprising that it's causing an issue now. This was added in commit f2841d377060 ("Add support for ymodem protocol (loady command). Patch by Stefano Babic, 29 Mar 2006").
Not just 255 and zero but any time "blk == 255 - cblk" then your new condition will be true where the original condition was false. Is that really intentional? Is this from reviewing documentation or something? What documents? Or is it from testing?
regards, dan carpenter
On Tue, Feb 06, 2024 at 09:05:33PM +0800, jihongbin wrote:
It may be that there are relatively few people using this function, or the length of the transmitted data is less than 128byte * 255 byte, so the triggering conditions for the above problems are not encountered;
I actually discovered this problem during testing. Continuous transmission always fails at block 255.
The following is the protocol's definition of blk cblk, cblk = 255 - blk The original content of the agreement (https://www.menie.org/georges/embedded/xmodem_specification.html) is as follows: -------- 3. MESSAGE BLOCK LEVEL PROTOCOL Each block of the transfer looks like: <SOH><blk #><255-blk #><--128 data bytes--><cksum> in which: <SOH> = 01 hex <blk #> = binary number, starts at 01 increments by 1, and wraps 0FFH to 00H (not to 01) <255-blk #> = blk # after going thru 8080 "CMA" instr, i.e. each bit complemented in the 8-bit block number. Formally, this is the "ones complement".
<cksum> = the sum of the data bytes only. Toss any carry.
Thanks, this is good information. Unfortunately, your patch is not correct.
Originally you wrote "When the blk sequence number is 255 and cblk is 0, the original XOR condition produces a result of 0, and the judgment condition will be unsuccessful."
If blk is 255 the cblk should be zero as you say.
common/xyzModem.c 375 /* Validate the message */ 376 if ((xyz.blk ^ xyz.cblk) != (unsigned char) 0xFF) 377 { 378 ZM_DEBUG (zm_dprintf 379 ("Framing error - blk: %x/%x/%x\n", xyz.blk, xyz.cblk, 380 (xyz.blk ^ xyz.cblk)));
0xff ^ 0 is equal to 0xFF so it won't print an error. Good!
With your patch, there is an issue with type promotion so the condition is always true and it will mark everything as a "Framming error".
- if ((xyz.blk ^ xyz.cblk) != (unsigned char) 0xFF) + if (~xyz.blk != xyz.cblk)
Both xyz.blk and xyz.cblk are type unsigned char. If you take the ~ of an unsigned char it's going to be 0xffffffXX where XX are the a variable bits. That's never going to be equal to xyz.cblk.
You could truncate it to char like this:
+ if ((unsigned char)~xyz.blk != xyz.cblk)
But then it works exactly the same as the original condition.
regards, dan carpenter
On Tue, Feb 06, 2024 at 09:05:33PM +0800, jihongbin wrote:
It may be that there are relatively few people using this function, or the length of the transmitted data is less than 128byte * 255 byte, so the triggering conditions for the above problems are not encountered;
I actually discovered this problem during testing. Continuous transmission always fails at block 255.
The following is the protocol's definition of blk cblk, cblk = 255 - blk The original content of the agreement (https://www.menie.org/georges/embedded/xmodem_specification.html) is as follows: -------- 3. MESSAGE BLOCK LEVEL PROTOCOL Each block of the transfer looks like: <SOH><blk #><255-blk #><--128 data bytes--><cksum> in which: <SOH> = 01 hex <blk #> = binary number, starts at 01 increments by 1, and wraps 0FFH to 00H (not to 01) <255-blk #> = blk # after going thru 8080 "CMA" instr, i.e. each bit complemented in the 8-bit block number. Formally, this is the "ones complement".
<cksum> = the sum of the data bytes only. Toss any carry.
Please repost the patch with these kind of details in the commit message, thanks.
participants (4)
-
Dan Carpenter -
Hongbin Ji -
jihongbin
-
Tom Rini